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

Fail on unexpected Javascript console errors #4288

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

kalikiana
Copy link
Member

@kalikiana kalikiana commented Oct 11, 2021

The function merely returns a truthy value. Let's fail always without relying on the caller. This means we will see errors in all cases, not just when something else happens to fail.

I'm also dropping a comment claiming the function aborts.

See: https://progress.opensuse.org/issues/98952

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I am not convinced to move the Test::More calls into the function as I learned that test failure feedback works best if the scope of such calls is the top-level test code and everything on a lower level should return the errors as boolean result

t/lib/OpenQA/SeleniumTest.pm Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor

The advantage of this approach is that we cannot forget to actually use the return value but I also get @okurz 's point. It would be great if Perl had a [[nodiscard]] attribute like C++ but it seems like it hasn't.

@kalikiana
Copy link
Member Author

No idea what this is:

[18:23:14] t/full-stack.t .. 1/? Could not obtain new session: unknown error: Chrome failed to start: crashed.
  (chrome not reachable)
  (The process started from chrome location /usr/lib64/chromium/chrome is no longer running, so ChromeDriver is assuming that Chrome has crashed.)
  (Driver info: chromedriver=94.0.4606.71 (1d32b169326531e600d836bd395efc1b53d0f6ef-refs/branch-heads/4606@{#1256}),platform=Linux 4.15.0-1110-aws x86_64) at /home/squamata/project/t/lib/OpenQA/SeleniumTest.pm line 98.
[18:23:14] t/full-stack.t .. 2/? # Tests were run but no plan was declared and done_testing() was not seen.
[18:23:14] t/full-stack.t ..      Dubious, test returned 254 (wstat 65024, 0xfe00)
All 2 subtests passed

@kalikiana kalikiana marked this pull request as ready for review October 12, 2021 07:25
@okurz
Copy link
Member

okurz commented Oct 12, 2021

chrome also crashed for me in #4289

@okurz
Copy link
Member

okurz commented Oct 12, 2021

chrome problems worked around in #4291 , I had already approved. Approval still holds :)

@kalikiana kalikiana force-pushed the full_stack_t_console_errors branch 2 times, most recently from 8f4cf0c to 330ce21 Compare October 13, 2021 07:17
t/full-stack.t Outdated Show resolved Hide resolved
t/lib/OpenQA/Test/FullstackUtils.pm Outdated Show resolved Hide resolved
@kalikiana kalikiana force-pushed the full_stack_t_console_errors branch 3 times, most recently from 968e0cb to 3ca9299 Compare October 22, 2021 08:59
@os-autoinst os-autoinst deleted a comment from mergify bot Oct 22, 2021
@os-autoinst os-autoinst deleted a comment from mergify bot Oct 22, 2021
Martchus
Martchus previously approved these changes Oct 22, 2021
@Martchus Martchus dismissed their stale review October 22, 2021 10:57

Looks like the test failures are actually related.

@Martchus
Copy link
Contributor

Maybe this warning should be ignored?

Bailout called.  Further testing stopped:  Job 1 produced the wrong results
FAILED--Further testing stopped: Job 1 produced the wrong results
Calling retry hook ./tools/delete-coverdb-folder
Deleting cover_db_fullstack/
Retry 7 of 15 …
[09:08:22] t/full-stack.t .. 32/?     # Unexpected Javascript console errors: [
    #   {
    #     level     => "SEVERE",
    #     message   => "http://localhost:9526/asset/2f2b2ee008/test_result.js 289 WebSocket connection to 'ws://localhost:9528/liveviewhandler/tests/1/developer/ws-proxy/status' failed: Close received after close",
    #     source    => "network",
    #     timestamp => 1634893738701,
    #   },
    # ]

    #   Failed test 'No unexected js warnings'

@kalikiana
Copy link
Member Author

This looks like the symptom of the json file never being written:

not ok 60 - developer session locked for other developers
Subtest: developer session locked for other developers
[debug] [pid:2726] Unable to read result-shutdown.json: Can't open file "/tmp/zm6SQvzhGB/full-stack.d/openqa/pool/1/testresults/result-shutdown.json": No such file or directory at /usr/lib/perl5/vendor_perl/5.26.1/Mojo/File.pm line 132.

@kalikiana
Copy link
Member Author

Maybe this warning should be ignored?

Bailout called.  Further testing stopped:  Job 1 produced the wrong results
FAILED--Further testing stopped: Job 1 produced the wrong results
Calling retry hook ./tools/delete-coverdb-folder
Deleting cover_db_fullstack/
Retry 7 of 15 …
[09:08:22] t/full-stack.t .. 32/?     # Unexpected Javascript console errors: [
    #   {
    #     level     => "SEVERE",
    #     message   => "http://localhost:9526/asset/2f2b2ee008/test_result.js 289 WebSocket connection to 'ws://localhost:9528/liveviewhandler/tests/1/developer/ws-proxy/status' failed: Close received after close",
    #     source    => "network",
    #     timestamp => 1634893738701,
    #   },
    # ]

    #   Failed test 'No unexected js warnings'

This is the one I'd added before but dropped when making all fatal. Which means I'm adding it back now.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #4288 (48664aa) into master (ba3ae18) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4288      +/-   ##
==========================================
- Coverage   97.94%   97.94%   -0.01%     
==========================================
  Files         371      371              
  Lines       33705    33704       -1     
==========================================
- Hits        33012    33011       -1     
  Misses        693      693              
Impacted Files Coverage Δ
t/full-stack.t 96.96% <100.00%> (ø)
t/lib/OpenQA/SeleniumTest.pm 97.46% <100.00%> (-0.02%) ⬇️
t/lib/OpenQA/Test/FullstackUtils.pm 100.00% <100.00%> (ø)
t/ui/15-comments.t 100.00% <100.00%> (ø)
t/ui/16-tests_dependencies.t 100.00% <100.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 ba3ae18...48664aa. Read the comment docs.

@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2021

This pull request is now in conflicts. Could you fix it? 🙏

kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 9, 2021
Whilst os-autoinst#4352 doesn't work I'm opening this to provide a reference for the stability of os-autoinst#4288.
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 9, 2021
Whilst os-autoinst#4352 doesn't work I'm opening this to provide a reference for the stability of os-autoinst#4288.
t/full-stack.t Show resolved Hide resolved
t/lib/OpenQA/Test/FullstackUtils.pm Outdated Show resolved Hide resolved
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 10, 2021
Whilst os-autoinst#4352 doesn't work I'm opening this to provide a reference for the stability of os-autoinst#4288.
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 10, 2021
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 10, 2021
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 10, 2021
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 12, 2021
@kalikiana kalikiana force-pushed the full_stack_t_console_errors branch 2 times, most recently from b16843b to 39f790b Compare November 12, 2021 20:01
kalikiana added a commit to kalikiana/openQA that referenced this pull request Nov 12, 2021
Makefile Outdated Show resolved Hide resolved
@kalikiana kalikiana force-pushed the full_stack_t_console_errors branch 4 times, most recently from 34f85e8 to 2b9f274 Compare November 16, 2021 19:01
The function merely returns a truthy value. Let's fail always without
relying on the caller. This means we will see errors in all cases, not
just when something else happens to fail.

I'm also dropping a comment claiming the function aborts.
@mergify mergify bot merged commit eab2bad into os-autoinst:master Nov 22, 2021
openqabot pushed a commit to openqabot/openQA that referenced this pull request Nov 23, 2021
commit eab2bad
Merge: 4c2ce7a 48664aa
Author:     mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
AuthorDate: Mon Nov 22 06:27:15 2021 +0000
Commit:     GitHub <noreply@github.com>
CommitDate: Mon Nov 22 06:27:15 2021 +0000

    Merge pull request os-autoinst#4288 from kalikiana/full_stack_t_console_errors

    Fail on unexpected Javascript console errors
okurz added a commit to okurz/openQA that referenced this pull request Nov 30, 2021
This fixes an error introduced with
os-autoinst#4288
where an additional "if"-expression was introduced looking for two
alternatives "javascript" or "network" for the error source which means
that we never enter the next elsif-sections for such errors.

This commit simply removes the distinction of javascript error sources
as we can be strict enough with our individual regex's looking for
specific errors to skip.

Related progress issue: https://progress.opensuse.org/issues/102578
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.

3 participants