Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Correct PR branch detection in code coverage #12615

Merged
merged 5 commits into from Sep 21, 2018
Merged

Conversation

marcoabreu
Copy link
Contributor

@marcoabreu marcoabreu commented Sep 20, 2018

The branch detection in the code coverage script was broken because a non-existing environment variable in groovy is considered null opposed to other documentation I've found that states it as being an empty string.

A CI run to test the branch detection in a non-PR job is available below (that's why I created the branch in the main repository instead of my own fork).

A CI run to test the branch detection in a PR job is the regular PR validation.

Accept criteria is both reports referencing the same commit hash.

Background for the change was that during my testing I used git log while I switched to rev-parse during last minute. Since CI automatically moves the currently checked out commit to the right one, rev-parse is actually working for PR-merge as well as for Branch validation.

@project-bot project-bot bot added this to new/unassigned/reopened prs in Apache Cordova: project-bot automation testing Sep 20, 2018
@janpio janpio removed this from new/unassigned/reopened prs in Apache Cordova: project-bot automation testing Sep 20, 2018
@marcoabreu
Copy link
Contributor Author

Closing to trigger branch job

@marcoabreu marcoabreu closed this Sep 20, 2018
@marcoabreu
Copy link
Contributor Author

I have verified that this change d724c8c works for PRs that have been submitted from the main repo as well as from a user fork.

Branch validation is pending at http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/fix-testcoverage-report/2/pipeline

@marcoabreu marcoabreu reopened this Sep 20, 2018
@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Sep 20, 2018
@marcoabreu
Copy link
Contributor Author

Branch validation successful. Everything's working as expected now :)

@marcoabreu marcoabreu changed the title [WIP] Correct PR branch detection in code coverage Correct PR branch detection in code coverage Sep 20, 2018
@marcoabreu
Copy link
Contributor Author

It seems like it just broke again. Please don't merge

@marcoabreu
Copy link
Contributor Author

marcoabreu commented Sep 20, 2018

Ah! I found something:

These two runs reference the same commit d724c8c. But the SCM output is different:

http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12615/2/pipeline/980/

Cloning the remote Git repository

Cloning with configured refspecs honoured and without tags

Cloning repository https://github.com/apache/incubator-mxnet.git

 > git init /home/jenkins_slave/workspace/ut-clojure-cpu # timeout=10

Fetching upstream changes from https://github.com/apache/incubator-mxnet.git

 > git --version # timeout=10

using GIT_ASKPASS to set credentials 

 > git fetch --no-tags --progress https://github.com/apache/incubator-mxnet.git +refs/pull/12615/head:refs/remotes/origin/PR-12615 +refs/heads/master:refs/remotes/origin/master

 > git config remote.origin.url https://github.com/apache/incubator-mxnet.git # timeout=10

 > git config --add remote.origin.fetch +refs/pull/12615/head:refs/remotes/origin/PR-12615 # timeout=10

 > git config --add remote.origin.fetch +refs/heads/master:refs/remotes/origin/master # timeout=10

 > git config remote.origin.url https://github.com/apache/incubator-mxnet.git # timeout=10

Fetching without tags

Fetching upstream changes from https://github.com/apache/incubator-mxnet.git

using GIT_ASKPASS to set credentials 

 > git fetch --no-tags --progress https://github.com/apache/incubator-mxnet.git +refs/pull/12615/head:refs/remotes/origin/PR-12615 +refs/heads/master:refs/remotes/origin/master

Merging remotes/origin/master commit 7797584450186d36e52c902c3b606f4b4676e0a3 into PR head commit d724c8cbffb90bb4ba64397eadaa1ef667f88ccc

 > git config core.sparsecheckout # timeout=10

 > git checkout -f d724c8cbffb90bb4ba64397eadaa1ef667f88ccc

 > git merge 7797584450186d36e52c902c3b606f4b4676e0a3 # timeout=10

 > git rev-parse HEAD^{commit} # timeout=10

Merge succeeded, producing d724c8cbffb90bb4ba64397eadaa1ef667f88ccc

Checking out Revision d724c8cbffb90bb4ba64397eadaa1ef667f88ccc (PR-12615)

 > git config core.sparsecheckout # timeout=10

 > git checkout -f d724c8cbffb90bb4ba64397eadaa1ef667f88ccc

Commit message: "Always use latest commit"

http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12615/3/pipeline/980

Cloning the remote Git repository

Cloning with configured refspecs honoured and without tags

Cloning repository https://github.com/apache/incubator-mxnet.git

 > git init /home/jenkins_slave/workspace/ut-clojure-cpu # timeout=10

Fetching upstream changes from https://github.com/apache/incubator-mxnet.git

 > git --version # timeout=10

using GIT_ASKPASS to set credentials 

 > git fetch --no-tags --progress https://github.com/apache/incubator-mxnet.git +refs/pull/12615/head:refs/remotes/origin/PR-12615 +refs/heads/master:refs/remotes/origin/master

 > git config remote.origin.url https://github.com/apache/incubator-mxnet.git # timeout=10

 > git config --add remote.origin.fetch +refs/pull/12615/head:refs/remotes/origin/PR-12615 # timeout=10

 > git config --add remote.origin.fetch +refs/heads/master:refs/remotes/origin/master # timeout=10

 > git config remote.origin.url https://github.com/apache/incubator-mxnet.git # timeout=10

Fetching without tags

Fetching upstream changes from https://github.com/apache/incubator-mxnet.git

using GIT_ASKPASS to set credentials 

 > git fetch --no-tags --progress https://github.com/apache/incubator-mxnet.git +refs/pull/12615/head:refs/remotes/origin/PR-12615 +refs/heads/master:refs/remotes/origin/master

Merging remotes/origin/master commit 3401e6e116615730882b9bede5b6abbb51b9a547 into PR head commit d724c8cbffb90bb4ba64397eadaa1ef667f88ccc

 > git config core.sparsecheckout # timeout=10

 > git checkout -f d724c8cbffb90bb4ba64397eadaa1ef667f88ccc

 > git merge 3401e6e116615730882b9bede5b6abbb51b9a547 # timeout=10

 > git rev-parse HEAD^{commit} # timeout=10

Merge succeeded, producing 2aeb7ad9247d0aded866fb8990572addee451c39

Checking out Revision 2aeb7ad9247d0aded866fb8990572addee451c39 (PR-12615)

 > git config core.sparsecheckout # timeout=10

 > git checkout -f 2aeb7ad9247d0aded866fb8990572addee451c39

Commit message: "Merge commit '3401e6e116615730882b9bede5b6abbb51b9a547' into HEAD"

The key difference here is that during the first run, my PRs base was the same as the HEAD of the master branch: 7797584 and my branch was 2 commits ahead and 0 behind. Thus, no merge was necessary but a simple fast-forward was applied.

During the second run, 3401e6e was pushed to the master branch and thus my branch was 2 commits ahead and 1 behind. This requires a merge commit.

This means that I don't have to make this logic conditional on whether the current run is part of a PR or a regular branch but rather parse the current contexts' HEAD and check whether it matches "Merge commit '$HASH$' into HEAD".

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Sep 20, 2018
@marcoabreu marcoabreu closed this Sep 20, 2018
@marcoabreu marcoabreu reopened this Sep 20, 2018
@marcoabreu marcoabreu force-pushed the fix-testcoverage-report branch 2 times, most recently from 077abb1 to b5545b1 Compare September 21, 2018 09:14
@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Sep 21, 2018
@marcoabreu marcoabreu removed the pr-work-in-progress PR is still work in progress label Sep 21, 2018
@marcoabreu marcoabreu merged commit 1045140 into master Sep 21, 2018
@marcoabreu marcoabreu deleted the fix-testcoverage-report branch September 22, 2018 11:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants