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

Upgraded SIL.BuildTasks used in .proj file. Removed dependency on it from SIL.Windows.Forms #1363

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

tombogle
Copy link
Contributor

@tombogle tombogle commented Nov 8, 2024

This change is Reviewable

@tombogle tombogle added the dependencies Pull requests that update a dependency file label Nov 8, 2024
@tombogle tombogle self-assigned this Nov 8, 2024
Copy link

github-actions bot commented Nov 8, 2024

LibPalaso Tests

    36 files   -     9      36 suites   - 9   8m 47s ⏱️ - 3m 8s
 4 132 tests  -   698   3 906 ✅  -   693  225 💤  -  6  1 ❌ +1 
11 152 runs   - 2 856  10 595 ✅  - 2 786  556 💤  - 71  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 75bb705. ± Comparison against base commit de37405.

This pull request removes 698 tests.
SIL.DblBundle.Tests.BundleTests ‑ LanguageIso_LanguageCodeSpecifiedInMetadata_ReturnsLanguageCode
SIL.DblBundle.Tests.BundleTests ‑ LanguageIso_NoLanguageCodeSpecified_ReturnsCodeForUnlistedLanguage
SIL.DblBundle.Tests.DblMetadataCopyrightTests ‑ ConstructorFromString_SimpleString_StatementHasSingleNode
SIL.DblBundle.Tests.DblMetadataCopyrightTests ‑ ConstructorFromString_XHtmlStringWithTwoParagraphs_StatementHasTwoNodesWithCorrectContent
SIL.DblBundle.Tests.DblMetadataCopyrightTests ‑ ToString_AsXHtml_ResultIsStatementXhtml
SIL.DblBundle.Tests.DblMetadataCopyrightTests ‑ ToString_SeparatorSpecified_SpecifiedSeparatorUsed
SIL.DblBundle.Tests.DblMetadataTests ‑ GetCopyrightStatement
SIL.DblBundle.Tests.DblMetadataTests ‑ GetDateArchived
SIL.DblBundle.Tests.DblMetadataTests ‑ GetId
SIL.DblBundle.Tests.DblMetadataTests ‑ GetLanguageIso
…

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

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

I see a single failing test here that I haven't seen be flaky before. Have you looked into this? I wonder if that would alleviate the problem of all the missing tests.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tombogle)

@tombogle
Copy link
Contributor Author

I see a single failing test here that I haven't seen be flaky before. Have you looked into this? I wonder if that would alleviate the problem of all the missing tests.

I didn't even bother looking. I figured I'd wait until the flaky test issue was resolved for good first.

@josephmyers
Copy link
Collaborator

We can hope it gets fixed, but we're still seeing flaky tests, even when running each framework on its own machine. At the very least, my PR reduces server build time. It should also help with flakiness, but it won't go away. Some of these tests are just inherently flaky, unrelated to framework, e.g. testing the clipboard. In their case, we just need to mark them with Retry. I've done this for the one I'm seeing on my PR, but it'll be case by case, whether we can improve it or have to mark it with Retry.

Copy link

github-actions bot commented Nov 14, 2024

Palaso Tests

     3 files  ±    0       3 suites  ±0   17m 17s ⏱️ + 8m 55s
 4 837 tests ±    0   4 606 ✅ ±    0  231 💤 ±  0  0 ❌ ±0 
14 022 runs  +4 778  13 395 ✅ +4 500  627 💤 +278  0 ❌ ±0 

Results for commit 3aaa08d. ± Comparison against base commit 7d09928.

♻️ This comment has been updated with latest results.

@tombogle
Copy link
Contributor Author

We can hope it gets fixed, but we're still seeing flaky tests, even when running each framework on its own machine. At the very least, my PR reduces server build time. It should also help with flakiness, but it won't go away. Some of these tests are just inherently flaky, unrelated to framework, e.g. testing the clipboard. In their case, we just need to mark them with Retry. I've done this for the one I'm seeing on my PR, but it'll be case by case, whether we can improve it or have to mark it with Retry.

Neither this text DLL nor the SUT reference SIL.BuildTasks, and this test passes for me locally, so it feels unlikely that it could be caused by this change. I did a tiny code simplification. Let's see if it magically passes this time.

@tombogle
Copy link
Contributor Author

After merging master, all the tests pass. However, I'm also seeing a similar failure in #1364, so I suspect it is another instance of flakiness. Not 100% sure, but it seems to be coming from tests of LocateInProgramFilesFolder. I would think that if we're running up against a permissions problem, it would be failing consistently. But those tests do seem at least somewhat susceptible to failures on build agents.

@tombogle tombogle merged commit 7004a47 into master Dec 3, 2024
6 of 7 checks passed
@tombogle tombogle deleted the fixed-buldtasks-dependency branch December 3, 2024 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants