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

[Jenkins] remove some duplicate test phases #3534

Merged
merged 2 commits into from
Aug 24, 2019

Conversation

jonathanpeppers
Copy link
Member

As we slowly migrate to Azure DevOps, there are some duplicate phases
we are running on Jenkins that mostly produce noise, such as:

  • MSBuild Tests - we commonly get failures related to NuGet
  • BCL Tests - enough said...
  • Networking-related issues on APK-based tests

For now we could replace the call:

make run-all-tests

To run a subset:

make run-performance-tests

This will keep the existing plots we have working. OSS contributors
will still have access to builds, etc.

Copy link
Member

@pjcollins pjcollins left a comment

Choose a reason for hiding this comment

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

Makes sense to me, though plotting was skipped due to this being a PR build. I suppose there's a chance we could break that in some way when it's merged?

@jonathanpeppers
Copy link
Member Author

Hmm, I guess we have to merge it to know?

@pjcollins
Copy link
Member

@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Aug 22, 2019
@jonathanpeppers
Copy link
Member Author

I'm not sure the plots are working, but it didn't give much info either:

[Pipeline] stage
[Pipeline] { (Plot build & test metrics)
[Pipeline] retry
[Pipeline] {
[Pipeline] timeout
[2019-08-22T22:12:22.137Z] Timeout set to expire in 30 sec
[Pipeline] {
[Pipeline] dir
[2019-08-22T22:12:22.157Z] Running in /Users/builder/jenkins/workspace/xamarin-android-pr-pipeline-release/xamarin-android
[Pipeline] {
[Pipeline] plot
[Pipeline] plot
[Pipeline] plot
[Pipeline] plot
[Pipeline] plot
[Pipeline] plot
[Pipeline] plot
[Pipeline] plot
[Pipeline] }
[Pipeline] // dir
[Pipeline] }
[Pipeline] // timeout
[Pipeline] }
[Pipeline] // retry
[Pipeline] }
[Pipeline] // stage
[Pipeline] echo
[2019-08-22T22:12:22.806Z] Stage result: Plot build & test metrics: SUCCESS

@jonathanpeppers
Copy link
Member Author

The CSV files for the plots are produced:

image

Maybe we will still have to merge to know if they work?

As we slowly migrate to Azure DevOps, there are some duplicate phases
we are running on Jenkins that mostly produce noise, such as:

* MSBuild Tests - we commonly get failures related to NuGet
* BCL Tests - enough said...
* Networking-related issues on APK-based tests

For now we could replace the call:

    make run-all-tests

To run a subset:

    make run-performance-tests

This will keep the existing plots we have working. OSS contributors
will still have access to builds, etc.

Other changes:

* The `run-performance-tests` target should run the APK-based
  performance tests.
* We need to add `Mono.Android-Tests` to the APK performance tests.

Update RunApkTests.targets
Makefile Outdated
@@ -176,7 +176,10 @@ run-apk-tests:
exit $$_r

run-performance-tests:
$(call MSBUILD_BINLOG,run-performance-tests,,Test) $(TEST_TARGETS) /t:RunPerformanceTests
_r=0 ; \
$(call MSBUILD_BINLOG,run-apk-tests,,Test) $(TEST_TARGETS) /t:RunApkTests /p:RunApkTestsTarget=RunPerformanceApkTests $(APK_TESTS_PROP) || _r=$$? ; \
Copy link
Member

Choose a reason for hiding this comment

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

Don't set _r=$$?. I know this is tempting, but it's only viable if you do this once. As soon as you do it a second time, you introduce the ability to clear $_r, because e.g. the second msbuild /t:RunPerformanceTests has an exit value of 0:

$(call MSBUILD_BINLOG,run-apk-tests,,Test) $(TEST_TARGETS) /t:RunApkTests /p:RunApkTestsTarget=RunPerformanceApkTests $(APK_TESTS_PROP) || _r=$$?
# sets _r=1
$(call MSBUILD_BINLOG,run-performance-tests,,Test) $(TEST_TARGETS) /t:RunPerformanceTests || _r = $$?
# sets _r=0
exit $$_r;  # look ma, no errors!

As such, you must ensure that you control the exit value:

$(call MSBUILD_BINLOG,run-apk-tests,,Test) $(TEST_TARGETS) /t:RunApkTests /p:RunApkTestsTarget=RunPerformanceApkTests $(APK_TESTS_PROP) || _r=1
# sets _r=1
$(call MSBUILD_BINLOG,run-performance-tests,,Test) $(TEST_TARGETS) /t:RunPerformanceTests || _r=1
# sets _r=1
exit $$_r;  # look ma, errors!

Makefile Outdated
$(call MSBUILD_BINLOG,run-performance-tests,,Test) $(TEST_TARGETS) /t:RunPerformanceTests
_r=0 ; \
$(call MSBUILD_BINLOG,run-apk-tests,,Test) $(TEST_TARGETS) /t:RunApkTests /p:RunApkTestsTarget=RunPerformanceApkTests $(APK_TESTS_PROP) || _r=$$? ; \
$(call MSBUILD_BINLOG,run-performance-tests,,Test) $(TEST_TARGETS) /t:RunPerformanceTests || _r = $$? ; \
Copy link
Member

@jonpryor jonpryor Aug 23, 2019

Choose a reason for hiding this comment

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

Setting aside the previous comment, the spaces around assignment here means that _r is not changed, and the overall pipeline has an exit value of 128:

$ _r=0; (exit 1) || _r = 1;
-bash: _r: command not found
$ echo $?
127

I'm not entirely sure what that'll do within a Makefile. The fact that the Jenkins PR build is green is not encouraging...though neither of these $(call MSBUILD_BINLOG) statements failed, which means _r = $$? wasn't even evaluated...

$ _r=0; (exit 0) || _r = 1;
$ echo $?
0

It turns out I copied another example in this file, which was also not correct...
@jonathanpeppers jonathanpeppers removed the do-not-merge PR should not be merged. label Aug 24, 2019
@jonpryor jonpryor merged commit ed597fb into dotnet:master Aug 24, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants