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

Enable dumpio when running the tests in order to have some useful debug data #18260

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman calixteman requested a review from timvandermeij June 16, 2024 18:53
@calixteman
Copy link
Contributor Author

It could help to figure out what's wrong in #18259.

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/34fcd127eefeaf9/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/cd69d4693491265/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/34fcd127eefeaf9/output.txt

Total script time: 28.57 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 12
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/34fcd127eefeaf9/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/cd69d4693491265/output.txt

Total script time: 44.23 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 5

Image differences available at: http://54.193.163.58:8877/cd69d4693491265/reftest-analyzer.html#web=eq.log

@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 17, 2024

Hm, I'm a bit on the fence about this. On the one hand it did already provide useful information and surfaced issues such as #18246 and the logs below, but on the other hand it also generates a lot of log spam for especially the integration tests which makes navigating the logs harder.

I'd feel better about this if we could tidy the logs up a bit first:

  • Preferably we fix those logs altogether, because for example logs like JavaScript error: http://127.0.0.1:62573/build/generic/web/viewer.mjs, line 3023: Error: Not implemented: #createBundleFallback already don't look particularly good (in that sense it proves the value of this setting though).
  • If that is difficult we could also choose to enable dumpio only for the reference tests and not for the unit/integration tests, at least for the time being until we figured out what's going on for Fix reference test "pr12564" #18259 (and maybe after that disable it again).

What do you think?

@calixteman
Copy link
Contributor Author

Yes there's some noise :(, especially those exceptions we've because of DecompressionStream (I must file a bug about that).
We should try to fix the createBundleFallback one.
That said in considering that it helped to fix one bug (in integration tests) and it's the best idea I've for the moment to be able to potentially catch something from this intermittent failure, I'd say we should do it.

@timvandermeij
Copy link
Contributor

It could help to figure out what's wrong in #18259.

Unfortunately I found out that this patch won't help with that particular issue; see #18259 (comment).

I'm still somewhat in favor of doing this, but ideally once the integration test logs are cleaned up at least a little bit, to avoid the logs becoming unwieldy to read through. I'll create dedicated issues for the most important/spammy ones because this patch did nicely surface those issues, so that in itself is really good.

@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 17, 2024

I have created the two issues above for the most important log spam problems here. I think once those are resolved we should be in a state where we can enable dumpio: true, which coming to think of it could also help with future patches because it'll surface new logs for PRs instead of them going by unnoticed as was the case until now.

@Snuffleupagus
Copy link
Collaborator

Can we perhaps revisit this one now, given the PRs that landed earlier today?

@timvandermeij
Copy link
Contributor

Yes, overall the logs should be in a much better state now that the two mentioned issues have been fixed. @calixteman Could you rebase this patch and trigger a new test run to verify that the logs should be in a good enough state now that we can enable this?

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @calixteman received. Current queue size: 2

Live output at: http://54.193.163.58:8877/32bb151d23ef0ea/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @calixteman received. Current queue size: 1

Live output at: http://54.241.84.105:8877/2495d314125b7a2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/2495d314125b7a2/output.txt

Total script time: 29.39 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 19
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/2495d314125b7a2/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/32bb151d23ef0ea/output.txt

Total script time: 42.76 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 6

Image differences available at: http://54.193.163.58:8877/32bb151d23ef0ea/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit c521351 into mozilla:master Jun 18, 2024
9 checks passed
@timvandermeij
Copy link
Contributor

This has clearly proven its value, and the logs are much more under control now. Good idea to enable this! I'll create a meta-issue to clean up the logs further, but it's not blocking landing this anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants