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

Gh-2711: conditionally run tests on windows #2716

Merged
merged 6 commits into from
Jul 25, 2022

Conversation

lb324567
Copy link
Member

@lb324567 lb324567 commented Jul 22, 2022

@lb324567
Copy link
Member Author

lb324567 commented Jul 22, 2022

Commit f96aa22 should pass (the tests anyway), as its only running the tests on Ubuntu (because of the keywords it's looking for).
Commit dd6a883 will probably fail because of issues with the tests running on windows, but does show that if the keywords are in the branch name the tests will run on windows.

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2022

Codecov Report

Merging #2716 (7398b99) into v2-alpha (8a43ebd) will decrease coverage by 0.20%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##             v2-alpha    #2716      +/-   ##
==============================================
- Coverage       65.73%   65.53%   -0.21%     
+ Complexity       2435     2268     -167     
==============================================
  Files             862      611     -251     
  Lines           27251    19188    -8063     
  Branches         3126     2259     -867     
==============================================
- Hits            17914    12575    -5339     
+ Misses           7980     5564    -2416     
+ Partials         1357     1049     -308     
Impacted Files Coverage Δ
...peration/mapper/generator/TextMapperGenerator.java
...chq/gaffer/flink/operation/handler/GafferSink.java
.../gchq/gaffer/mapstore/impl/GetElementsHandler.java
...ketches/quantiles/StringsSketchKryoSerializer.java
...edicate/user/FederatedGraphWriteUserPredicate.java
...byteEntity/ByteEntityAccumuloElementConverter.java
...java/uk/gov/gchq/gaffer/mapstore/impl/MapImpl.java
...rame/converter/property/impl/FreqMapConverter.java
...hq/gaffer/mapstore/impl/GetAdjacentIdsHandler.java
...ov/gchq/gaffer/accumulostore/utils/TableUtils.java
... and 242 more

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 a820490...7398b99. Read the comment docs.

@GCHQDeveloper314
Copy link
Member

I've just raised #2717 to cover the remaining Windows test failures. However, because it is not a priority issue, I don't think we should run this new Windows variant of CI for releases yet.
The next release is not planned to include the Windows fixes. If this PR were merged ahead of that release, the CI would fail when run on the release. We don't really want the CI to routinely fail on releases as this is misleading.

Also, under #2717 there will be extra changes required to the CI for Windows only (to install appropriate Hadoop libraries). If the release trigger were to remain disabled for now, it could then be enabled when #2717 is fixed (as there would be no more failures after this point).

@t92549
Copy link
Contributor

t92549 commented Jul 22, 2022

I've just raised #2717 to cover the remaining Windows test failures. However, because it is not a priority issue, I don't think we should run this new Windows variant of CI for releases yet. The next release is not planned to include the Windows fixes. If this PR were merged ahead of that release, the CI would fail when run on the release. We don't really want the CI to routinely fail on releases as this is misleading.

Also, under #2717 there will be extra changes required to the CI for Windows only (to install appropriate Hadoop libraries). If the release trigger were to remain disabled for now, it could then be enabled when #2717 is fixed (as there would be no more failures after this point).

So for now, this should only run when "windows" is in the branch name?

@GCHQDeveloper314
Copy link
Member

So for now, this should only run when "windows" is in the branch name?

Yes that's the idea, this way we can create branches and PRs to work on the Windows problems, but the releases will continue to build as usual and will pass. Then once the existing Windows issues are fixed, we can enable it for releases as well.

From a practical perspective, we could have release-disabled-for-now instead of release so that there's only a minor edit required to enable, that way this PR could be merged pretty much as it is, instead of taking the release bit out.

@lb324567
Copy link
Member Author

So for now, this should only run when "windows" is in the branch name?

Yes that's the idea, this way we can create branches and PRs to work on the Windows problems, but the releases will continue to build as usual and will pass. Then once the existing Windows issues are fixed, we can enable it for releases as well.

From a practical perspective, we could have release-disabled-for-now instead of release so that there's only a minor edit required to enable, that way this PR could be merged pretty much as it is, instead of taking the release bit out.

I've made the change to use release-disabled-for-now as you suggested :)

@GCHQDeveloper314
Copy link
Member

Because the branch for this PR contains the word 'windows', the tests are run on Windows and they fail because Windows isn't fully supported yet. This PR is ok to be merged despite the CI showing a failure.

@t92549 t92549 linked an issue Jul 25, 2022 that may be closed by this pull request
@t92549 t92549 merged commit 9f30700 into v2-alpha Jul 25, 2022
@t92549 t92549 deleted the gh-2711-conditionally-run-tests-on-windows branch July 25, 2022 09:04
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.

Add capability to run tests on Windows
4 participants