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.

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

[Merged by Bors] - Small test ux improvements #1704

Closed

Conversation

orndorffgrant
Copy link
Contributor

Hi, first time contributor :)

I was playing with getting the test262 test runner working and found a couple small things that I thought would've made my life a bit easier. I've split them into separate commits. Just let me know if you disagree with any and we can drop that commit. And of course, let me know of any changes you'd like to see :)

The changes are:

  • Adds details to the documentation on the --suite command to boa_tester. I was trying to pass a path starting with ./test262/ for a bit before I looked at the code and saw what it was doing.
  • Changes the individual test output when verbosity is > 1. Because the tests are run in parallel, the "Result" line for a given test was frequently not immediately after the "Started" line. This made it hard to determine which test had failed. The new output includes the test name in the result line, and also changes the format of all the individual-test-output lines to begin with the test name.
  • Adds a --disable-parallelism flag. Even with the adjustments to the test output, it was still a bit hard to follow. Running serially for small sub-suites produces output that can be more easily scanned IMO. I added this as a "disable" flag (as opposed to "enable parallelism") in order to maintain the current default of running in parallel.

@jedel1043 jedel1043 added enhancement New feature or request test Issues and PRs related to the tests. labels Nov 8, 2021
@jedel1043 jedel1043 added this to the v0.14.0 milestone Nov 8, 2021
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Good job! This should make it a lot easier to test for missing features and catching bugs :)

@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #1704 (ec953a4) into main (009bd09) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1704   +/-   ##
=======================================
  Coverage   50.85%   50.85%           
=======================================
  Files         199      199           
  Lines       17803    17803           
=======================================
  Hits         9053     9053           
  Misses       8750     8750           
Impacted Files Coverage Δ
boa_tester/src/main.rs 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 009bd09...ec953a4. Read the comment docs.

@orndorffgrant
Copy link
Contributor Author

Thanks!

And I just noticed I missed a formatting error. Just pushed an update, hopefully it's green now

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks good to me! In the future maybe we should have a log buffer in order to be able to parallelize and still get the correct output :)

@Razican
Copy link
Member

Razican commented Nov 9, 2021

bors r+

bors bot pushed a commit that referenced this pull request Nov 9, 2021
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

Hi, first time contributor :)

I was playing with getting the test262 test runner working and found a couple small things that I thought would've made my life a bit easier. I've split them into separate commits. Just let me know if you disagree with any and we can drop that commit. And of course, let me know of any changes you'd like to see :)

The changes are:
- Adds details to the documentation on the `--suite` command to `boa_tester`. I was trying to pass a path starting with `./test262/` for a bit before I looked at the code and saw what it was doing.
- Changes the individual test output when verbosity is > 1. Because the tests are run in parallel, the "Result" line for a given test was frequently not immediately after the "Started" line. This made it hard to determine which test had failed. The new output includes the test name in the result line, and also changes the format of all the individual-test-output lines to begin with the test name.
- Adds a `--disable-parallelism` flag. Even with the adjustments to the test output, it was still a bit hard to follow. Running serially for small sub-suites produces output that can be more easily scanned IMO. I added this as a "disable" flag (as opposed to "enable parallelism") in order to maintain the current default of running in parallel.


Co-authored-by: Grant Orndorff <grant@orndorff.me>
@bors
Copy link

bors bot commented Nov 9, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Small test ux improvements [Merged by Bors] - Small test ux improvements Nov 9, 2021
@bors bors bot closed this Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants