Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add base for GUI tests #2476

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add base for GUI tests #2476

wants to merge 3 commits into from

Conversation

GuillaumeGomez
Copy link
Member

As discussed with @ehuss, this PR adds the basics for adding more GUI tests. Just add more .goml files in the tests/gui folder.

This is the same GUI test framework used in rustdoc and docs.rs.

If you need any explanation on how it works, or if you have any question, please don't hesitate to ask.

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Nov 8, 2024
@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 2 times, most recently from a3f8cd0 to 1a2991f Compare November 8, 2024 14:39
@GuillaumeGomez GuillaumeGomez reopened this Nov 8, 2024
@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 5 times, most recently from ef31501 to 4b48db3 Compare November 8, 2024 16:46
@GuillaumeGomez
Copy link
Member Author

Finally fixed windows build. :')

@notriddle

This comment was marked as resolved.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Great idea, thanks! Updated.

notriddle
notriddle suggested changes Nov 22, 2024
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
tests/gui/runner.rs Outdated Show resolved Hide resolved
notriddle
notriddle reviewed Nov 22, 2024
View reviewed changes
CONTRIBUTING.md Outdated
cargo test --test gui -- --disable-headless-test
```

You can find the `browser-ui-test` package documentation on [its repository](https://github.com/GuillaumeGomez/browser-UI-test/blob/master/goml-script.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a lot of information on how those are written. Something like this?

The GUI tests are in the directory tests/gui in text files with the .goml extension. These tests are run using a node.js program called browser-ui-test. You can find documentation for this language on its repository.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks! Hard to know what is missing when you're too involved in something. ^^'

ehuss
ehuss reviewed Dec 12, 2024
View reviewed changes
.github/workflows/main.yml Outdated
Comment on lines 61 to 65
- name: Build and run tests (+ GUI)
if: matrix.os != 'windows-latest'
run: cargo test --locked --target ${{ matrix.target }} --test gui
- name: Build and run tests
if: matrix.os == 'windows-latest'
run: cargo test --locked --target ${{ matrix.target }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to disable the default tests on non-windows runners. cargo test --locked --target ${{ matrix.target }} --test gui only runs the GUI tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed! Good catch.

@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 5 times, most recently from b2c148a to a265652 Compare December 16, 2024 16:36
GuillaumeGomez
GuillaumeGomez commented Dec 16, 2024
View reviewed changes
.github/workflows/main.yml Outdated
run: npm install browser-ui-test@"${BROWSER_UI_TEST_VERSION}"
- name: Build and run tests (+ GUI)
if: matrix.os != 'windows-latest'
run: cargo test --locked --target ${{ matrix.target }} --test gui
- name: Build and run tests
run: cargo test --locked --target ${{ matrix.target }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I removed the if condition, the GUI test is now failing on linux. I have no clue what's going since the output seems to not be present for unknown reasons...

@GuillaumeGomez
Copy link
Member Author

Is there a way to still have all CIs run even if one failed?

@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 2 times, most recently from f1aa997 to 1cb0996 Compare December 16, 2024 22:53
@GuillaumeGomez
Copy link
Member Author

So GUI command is failing, but can't figure out why since there is no output. It's super weird.

notriddle
notriddle reviewed Dec 17, 2024
View reviewed changes
tests/gui/runner.rs Outdated

// Then we run the GUI tests on it.
let status = command.status().expect("failed to get command output");
assert!(status.success());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert!(status.success());
assert!(status.success(), "{status:?}",);

This should give us a little more information.

notriddle
notriddle reviewed Dec 18, 2024
View reviewed changes
tests/gui/runner.rs

let mut command = Command::new("npx");
command
.arg("browser-ui-test")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lacking anything else to try...

Suggested change
.arg("browser-ui-test")
.arg("browser-ui-test")
.args(["--debug", "--jobs", "1"])

Copy link
Contributor

@notriddle notriddle Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GuillaumeGomez

Error: Failed to launch the browser process!
[8316:8316:1219/155437.724812:FATAL:zygote_host_impl_linux.cc(126)] No usable sandbox! Update your kernel or see https://chromium.googlesource.com/chromium/src/+/main/docs/linux/suid_sandbox_development.md for more information on developing with the SUID sandbox. If you want to live dangerously and need an immediate workaround, you can try using --no-sandbox.

Okay, then. Looks like the solution is to turn off the sandbox (at least in CI, which is already isolated into containers).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to rustfmt this file. Tabs aren't used for indentation here.

@GuillaumeGomez
Copy link
Member Author

CI passed! \o/

Thanks so much @notriddle !

@binhle113 binhle113 mentioned this pull request Dec 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

@binhle113 binhle113 binhle113 approved these changes

@notriddle notriddle notriddle approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GuillaumeGomez @notriddle @ehuss @binhle113 @rustbot