Skip to content

fix(report): add global QUnit errors to reporter #6

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
merged 3 commits into from
Feb 16, 2025
Merged

fix(report): add global QUnit errors to reporter #6

merged 3 commits into from
Feb 16, 2025

Conversation

timmywil
Copy link
Member

  • Add errors from the QUnit error event to the list of error reports.
  • If the runEnd failure count is greater than 0, but no errors are in the pending list, report a global failure instead and fail the test run.

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I wanted to test it, so I pushed a new branch https://github.com/jquery/jquery-test-runner/tree/globals-broken-test where I pointed the jQuery submodule to a commit with a broken QUnit.test() with just a title and no test body: https://github.com/jquery/jquery/tree/broken-test.

I submitted a test PR to have the actions run: #7. Unfortunately, jQuery tests still passed: https://github.com/jquery/jquery-test-runner/actions/runs/13328747726/job/37227795585?pr=7#step:6:76

1 failed. 1130 passed. 0 skipped.
All tests passed!
Cleaning up...

Is testing set up properly in this repo? It should be using the local jtr, otherwise we're not really testing anything.

@timmywil
Copy link
Member Author

See my comment on your PR: #7 (comment)

@mgol
Copy link
Member

mgol commented Feb 14, 2025

My reply: #7 (comment)

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Tested it now and it works, thanks!

I'll approve but I wonder if we can improve it further. The current log is:

Tests finished in 47s 304ms at http://localhost:44603/test/ in Chrome (headless) (cf6363d09d)...
1 failed. 1129 passed. 0 skipped.
....................................................................................................................................................................................................................................................

Tests finished in 55s 661ms at http://localhost:44603/test/ in Firefox (headless) (6e4b5bdb1e)...
1 failed. 1130 passed. 0 skipped.

2 tests failed.

1. TypeError: You must provide a callback to QUnit.test("test with a title and without a body")
    at new Test (http://localhost:44603/external/qunit/qunit.js:2533:13)
    at addTest (http://localhost:44603/external/qunit/qunit.js:3270:19)
    at Object.test (http://localhost:44603/external/qunit/qunit.js:3287:5)
    at http://localhost:44603/test/unit/dimensions.js:371:7
    at http://localhost:44603/test/unit/dimensions.js:801:4

2. Test@http://localhost:44603/external/qunit/qunit.js:2533:13
addTest@http://localhost:44603/external/qunit/qunit.js:3270:19
test@http://localhost:44603/external/qunit/qunit.js:3287:12
@http://localhost:44603/test/unit/dimensions.js:371:7
@http://localhost:44603/test/unit/dimensions.js:801:4

Cleaning up...
Error: Process completed with exit code 1.

The second stack comes from Firefox, I think, and there's no message there. I think Firefox doesn't include the message in error.stack.

This is not critical as the message does appear earlier but if you found an elegant way to fix it, that'd be great.

@timmywil
Copy link
Member Author

I didn't notice that about Firefox. I pushed a change to include the message in the final report as well. It'll be redundant in Chrome, but that's fine.

$ npm run test:unit -- --headless -b firefox -f module=dimensions

> [email protected] test:unit
> jtr -m test/middleware-mockserver.cjs --headless -b firefox -f module=dimensions

Starting test run 18f52c6a90...


Error: You must provide a callback to QUnit.test("test with a title and without a body")
Test@http://localhost:50914/external/qunit/qunit.js:2533:13
addTest@http://localhost:50914/external/qunit/qunit.js:3270:19
test@http://localhost:50914/external/qunit/qunit.js:3287:12
@http://localhost:50914/test/unit/dimensions.js:371:7
@http://localhost:50914/test/unit/dimensions.js:801:4

.............

Tests finished in 216ms at http://localhost:50914/test/?module=dimensions in Firefox (headless) (5e17568d77)...
1 failed. 14 passed. 0 skipped.

1 test failed.

1. You must provide a callback to QUnit.test("test with a title and without a body")
Test@http://localhost:50914/external/qunit/qunit.js:2533:13
addTest@http://localhost:50914/external/qunit/qunit.js:3270:19
test@http://localhost:50914/external/qunit/qunit.js:3287:12
@http://localhost:50914/test/unit/dimensions.js:371:7
@http://localhost:50914/test/unit/dimensions.js:801:4

Cleaning up...

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@timmywil
Copy link
Member Author

timmywil commented Feb 15, 2025

Good idea. I'll add name, but I'll need to send it as its own property from the listener (stringifying an Error object results in {}).

@timmywil
Copy link
Member Author

timmywil commented Feb 15, 2025

@mgol Have a look at the latest. It now includes name in the error message, which I noticed means we're building the same thing as the first line of the Chrome error stack. So, I'm using it to clear out the duplicate message from the stack. Also, the final report is now reusing the built message from reportError.

Chrome

image

Firefox

image

@timmywil timmywil requested a review from mgol February 15, 2025 15:58
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

I've just tested the changes in Chrome, Firefox & Safari. The errors look great! LGTM.

@timmywil timmywil merged commit 7f12fad into main Feb 16, 2025
4 checks passed
@timmywil timmywil deleted the globals branch February 16, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants