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

change(web): drop 'dom' unit test concurrency when run in CI mode #13141

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Feb 6, 2025

@keymanapp-test-bot keymanapp-test-bot bot added this to the B18S1 milestone Feb 6, 2025
@@ -9,5 +9,6 @@ export default {
reporters: [
teamcityReporter(), /* custom-written, for CI-friendly reports */
sessionStabilityReporter({ciMode: true})
]
],
concurrency: 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that this setting was missing from the 'dom' subfolder while present in the 'integrated' subfolder. There's no clear cause behind the difference in state when tracing the origin of the lines via git blame. Concurrency is known to reduce stability a bit, so perhaps dropping it in CI mode may help with #12638.

@jahorton
Copy link
Contributor Author

jahorton commented Feb 6, 2025

Solid run on ba-win10-64-key-01: https://build.palaso.org/buildConfiguration/Keymanweb_TestPullRequests/522529.
Success with ba-win10-64-key-02: https://build.palaso.org/buildConfiguration/Keymanweb_TestPullRequests/522530.
And a solid set for ba-win10-64-s1-601 as well: https://build.palaso.org/buildConfiguration/Keymanweb_TestPullRequests/522532

With ba-win10-64-pp-602, we have a few tests missing... and I haven't found anything clear in the logs quite yet. Will need to inspect more thoroughly: https://build.palaso.org/buildConfiguration/Keymanweb_TestPullRequests/522531.

  • Surprisingly few are missing, so you'd think it would have to be near the run's end... but I don't see relevant log messages there.
  • The only distinct thing I've seen so far:
    [15:55:52] :	 [Step 2/6] [web/src/engine/predictive-text/worker-main/unit_tests] ## test:headless completed successfully
    [15:55:56]W:	 [Step 2/6] parse error: Invalid numeric literal at line 1, column 10
    [15:55:56]W:	 [Step 2/6] parse error: Invalid numeric literal at line 1, column 10
    [15:55:56] :	 [Step 2/6] Auto-skipping <test:browser> for unrelated CI test build
    
    Those "parse error" lines do not appear in the test runs that reached the full test count.

@jahorton
Copy link
Contributor Author

jahorton commented Feb 7, 2025

Wait a second...

15:55:56   Auto-skipping <test:browser> for unrelated CI test build

It's not skipped by the one with the full test count.

Also, on further inspection... test:libraries is being run; I thought that was supposed to be turned off for the main Web build. That'd have been part of #12866.

":engine/predictive-text=src/engine/predictive-text/worker-main Builds KMW's predictive text module" \

Ah, missed that - it's been bypassing #12866's script, and thus the predictive-text test filtering. Doesn't entirely explain by test-browser got skipped, admittedly.

That corresponds to these lines:

# If we are running a TeamCity test build, for now, only run BrowserStack
# tests when on a PR branch with a title including "(web)" or with the label
# test-browserstack. This is because the BrowserStack tests are currently
# unreliable, and the false positive failures are masking actual failures.
#
# We do not run BrowserStack tests on master, beta, or stable-x.y test
# builds.
if [[ $VERSION_ENVIRONMENT == test ]] && builder_has_action test:browser; then
if builder_pull_get_details; then
if ! ([[ $builder_pull_title =~ \(web\) ]] || builder_pull_has_label test-browserstack); then
echo "Auto-skipping $(builder_term test:browser) for unrelated CI test build"
exit 0
fi
fi
fi

But change(web) is part of this PR's title. Shouldn't that have prevented the 'skip'?

@mcdurdin mcdurdin changed the base branch from master to beta February 11, 2025 07:37
@jahorton jahorton merged commit 7feb496 into beta Feb 14, 2025
18 checks passed
@jahorton jahorton deleted the change/web/drop-CI-unit-test-concurrency branch February 14, 2025 07:56
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.193-beta

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

Successfully merging this pull request may close these issues.

3 participants