search

terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
214 stars 82 forks source link

Add test coverage to grid GUI #133

Open youngmit opened 3 years ago

youngmit commented 3 years ago

We whitelisted the grid GUI related code in the coverage reports because it is completely untested. That's bad and we feel bad. But also just needed to get the GUI done. Need to circle back around and add some meaningful testing so that it doesn't break all the time.

scottyak commented 3 years ago

Hi, I am interested in contributing. Is help still needed here?

A few clarifying questions from me:

youngmit commented 3 years ago

Yes, we could still use some help on this! I originally wrote the grid editor code, but as mentioned above, I don't have any experience testing GUIs, but know enough to know that I would have to do some head-scratching before I could do it "right". If you are familiar with any tools for that kind of thing, we are open to suggestions. Looks like autopilot uses DBus, so probably only supports linux-y environments. This is fine (especially since our CI is running under Linux), but the test would need skip gracefully on Windows, or other platforms that don't support it. Also, I'm not seeing any proper documentation of it, so it looks a bit sketchy. PyAutoGUI looks somewhat promising, though. Open to suggestions.

There isn't a whole lot of documentation of what the GUI should do, so if you need any guidance, let me know. Thanks for the interest in helping out!

scottyak commented 3 years ago

Thanks for the quick reply! I don't have any experience with desktop UI testing either (though I have worked with projects that have webdriver tests). I was looking for an interesting open source project to work on, this project sounds pretty cool, and I'm hoping to learn more about this project over time, starting here!

I'll admit I hadn't actually spent too much time looking into Autopilot myself; I wanted to make sure that this issue is still open. Yeah, Autopilot isn't looking so good if we can't test it on Windows (since it seems like most users use ARMI on Windows? Please correct me if I'm wrong), and I haven't found anything better myself, so I'm happy to start by trying out PyAutoGUI.

My first step would be to write a skeleton test case that verifies something simple, like changing "Num. Rings" to 1, selecting (i,j), clicking "Apply", and validating that the grid is indeed 2 x 2 (this would involve taking a screenshot of the 2 x 2 grid with some white background around it, saving it as an image file, and attempting to locate that image file using pyautogui.locateOnScreen). Once I get the skeleton code working, I can make a PR to start a discussion on how the code should be structured, before we dive into the concrete test cases. Once we get to that point, I'll definitely have more questions about what the GUI is expected to do.

Does this approach seem reasonable to you?

youngmit commented 3 years ago

That would definitely be a good place to start! We have used ARMI on windows and Linux, but yeah we mostly are one Windows; especially when we are tweaking inputs/using the GUI. That doesn't necessarily mean that the tests have to function on windows, since we have linux CI anyways, but they can't fail spectacularly in Windows. My guess is any Python-centric GUI test tools will be platform independent. Your approach seems reasonable, but again I've never tested GUIs before, so I couldn't tell you if it is the best way. For sure it would be nice if the tests can be run in a headless environment, so rendering offscreen in a way that you can interrogate color values or the like as you suggest, without having to draw to a screen.

This is also a rare opportunity for someone completely new to the space trying to poke ARMI, so if there is anything that you find confusing or non-obvious, we would welcome documentation improvements. The GUI is pretty much not documented at all

scottyak commented 3 years ago

Unfortunately PyAutoGUI isn't going to support headless mode any time soon: https://stackoverflow.com/a/42882644

But good news, I learned that wxPython itself comes with ways to write tests:

I think it might be better to start this way (as opposed to starting with PyAutoGUI). What do you think?

(Also, feel free to assign this issue to me.)

youngmit commented 3 years ago

You've been assigned! Headless or no, the main hope is that the tests can run on Travis. Not absolutely necessary, but a huge plus. I think starting with the built-in wxPython facilities makes a lot of sense. Thanks for signing on :+1:

scottyak commented 3 years ago

(Copying from a comment in PR#231)

Now that we have set up the scaffolding for local UI testing, I believe the next step is getting the test to pass in the Travis environment. This is not easy to do, because there isn't a display that allows me to see what the test is doing to the UI.

I think it's worth investing in a less frustrating debugging experience first. I propose to video capture the virtual display when the test is running, and store the video in some place we can access. This has several side benefits as well: Imagine if the GuiTestCase has a tearDown method that saves the screen recorded video to disk whenever the test fails. That would make bug reporting so much easier! I can also imagine using this tool to record demos (to make promotional materials or seek user feedback) and video tutorials (which would make the documentation superb).

I think I'm not too far from figuring out how to record the screen and save the video locally (Will make another PR when I get it to work). If this works, does it seem feasible to set up some location in S3 where test outputs can be dumped and viewed by non-Terrapower-employees?

There is some documentation here on how to upload artifacts from Travis to S3.

youngmit commented 3 years ago

I think you have some good ideas here! If you are still willing to keep pushing on this, it would be nice to get this stuff going under travis, for sure. Regarding S3, i think this could be an interesting thing to try, but I'm not sure if we will be able to stand up an S3 instance from TerraPower's side; the legal aspects would take some head scratching. However, I think linking travis to a personal aws account or the like would be reasonable, especially if we do not intend for that connection to be permanent (seems like we could hook them up, get travis configured properly, then unplug them?) Idk... thoughts? @ntouran?

scottyak commented 3 years ago

A hacky workaround I just thought of that doesn't involve AWS would be to base64 encode the video, print it to the logs, and I can download the raw logs and decode it. If Travis doesn't truncate the logs, that might just work...

youngmit commented 3 years ago

haha, madness... worth a try!

john-science commented 2 years ago

I wouldn't mind moving the GUI to a static JavaScript web browser tool. We could keep the current workflow, where it is spawned via the same commandline, and it doesn't require a running service.

But it seems like it would be easier to build and test a UI via browser. Just an idea.

scottyak commented 2 years ago

For one, UI testing in a browser would have a lot more resources on stack overflow and have better support in Travis :) It might be a good thing to do in the long term, provided users are fine with requiring a browser installation to run the GUI.

But wouldn't that involve rewriting everything that's currently written in wxPython, though? The HTML5 canvas API might be similar enough, but it would be in JavaScript/Typescript. It seems like a big change to me.

On Mon, Nov 29, 2021, 8:03 PM John Stilley @.***> wrote:

I wouldn't mind moving the GUI to a static JavaScript web browser tool. We could keep the current workflow, where it is spawned via the same commandline, and it doesn't require a running service.

But it seems like it would be easier to build and test a UI via browser. Just an idea.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/terrapower/armi/issues/133#issuecomment-982182783, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN6VYMOE37G24POSHYIC6LUOQPHBANCNFSM4QFM2FGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

john-science commented 2 years ago

@scottyak Yes, the switch to a browser-based solution would be a complete re-write. That's definitely the down-side.

I am just looking at this "test the GUI" PR that's been open for ages: https://github.com/terrapower/armi/pull/244

And we do want to be able to test the GUI. Is it unmaintainably hard to test as-is? I don't know. If so, that's a problem.

Just thinking aloud.

scottyak commented 2 years ago

Working on a notebook-based approach for rewriting the GUI. Stay tuned.

scottyak commented 2 years ago

We are considering adding NotebookJS (MIT license) as a dependency. It essentially allows Javascript callbacks to call Python functions, which is exactly what we want. From reading the code, I find the NotebookJS approach pretty elegant. Even though it's a one-person project, I feel okay with it, since the code looks rather straightforward (~500 lines). In the worst case scenario that some bug arises and the owner has abandoned the project, we could always copy the code into this repo along with its license header and figure it out.

john-science commented 2 years ago

We are considering adding NotebookJS (MIT license) as a dependency.

You're right that it's a very small project. But I like that it is well documented. It has an IEEE article and a blog post. It's MIT licensed, which I like. And as long as we only use it to import MIT licensed tools, we're good.

On balance, it seems like a good choice. Can't wait to see what happens!

scottyak commented 1 year ago

Sneak preview:

grid_gui_from_ascii_map

scottyak commented 1 year ago

grideditor-2022-10-27-1605.webm

john-science commented 1 year ago

That's awesome, @scottyak !

scottyak commented 1 year ago

Thanks! Here's a recording of the Fuel Path mode:

grideditor-2022-10-28-1536.webm

scottyak commented 1 year ago

Demonstrating an in-notebook grid editing workflow that takes advantage of the two-way data-binding capabilities.

In this demo, we show the following:

  1. Use armi.utils.asciimaps to load an ASCII map. Notice that the middle cells are initially "IC".
  2. Load the ASCII map into the HTML+SVG+d3 based grid editor GUI.
  3. Edit the grid using the GUI. Here, we edit a few middle cells to "US".
  4. Export the edited grid cell data back to the notebook.
  5. Use armi.utils.asciimaps to convert the grid cell data back into ASCII map. Observe that the ASCII map now has "US" in the middle, as intended in a grid editing workflow.

grideditor-2022-10-28-1752.webm