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

[check_stability] Improve "changed file" detection #5416

Merged
merged 1 commit into from
Apr 7, 2017

Conversation

jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Apr 6, 2017

@jgraham I'd like to call out the change from .. to ... in particular as
this has the potential to impact use cases beyond "TravisCI pull request."

The "check stability" script is intended to run only those tests that
have been effected by a given changeset. It calculates the relevant set
of test files by analyzing the git repository's history.

Previously, the script referenced an environment variable provided by
TravisCI named TRAVIS_COMMIT_RANGE when running for GitHub.com pull
requests. That variable is documented [1] as follows:

  • TRAVIS_COMMIT_RANGE: The range of commits that were included in the push
    or pull request. (Note that this is empty for builds triggered by the
    initial commit of a new branch.)

Beyond being somewhat imprecise, that value has been found to be
incorrect in some cases [2]. The definition leaves open the possibility
for race conditions resulting from changes to the target branch over the
course of the TravisCI build life cycle.

Update the "changed file" detection heuristic to instead use the
TRAVIS_BRANCH variable, whose definition is much more precise and less
susceptible to race conditions of the type described above. In addition,
update the operator used to specify git endpoints in the subsequent
invocation of git diff as this syntax is distinct from that used to
specify ranges [3].

[1] https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
[2] travis-ci/travis-ci#7578
[3] https://git-scm.com/docs/git-diff#_description

git diff [--options] <commit>..<commit> [--] [<path>...]
    This is synonymous to the previous form. If <commit> on one
    side is omitted, it will have the same effect as using HEAD
    instead.

git diff [--options] <commit>...<commit> [--] [<path>...]
    This form is to view the changes on the branch containing and
    up to the second <commit>, starting at a common ancestor of
    both <commit>. "git diff A...B" is equivalent to "git diff
    $(git-merge-base A B) B". You can omit any one of <commit>,
    which has the same effect as using HEAD instead.

Just in case if you are doing something exotic, it should be noted
that all of the <commit> in the above description, except in the
last two forms that use ".." notations, can be any <tree>.

For a more complete list of ways to spell <commit>, see
"SPECIFYING REVISIONS" section in gitrevisions(7). However, "diff"
is about comparing two endpoints, not ranges, and the range
notations ("<commit>..<commit>" and "<commit>...<commit>") do not
mean a range as defined in the "SPECIFYING RANGES" section in
gitrevisions(7).

This change is Reviewable

The "check stability" script is intended to run only those tests that
have been effected by a given changeset. It calculates the relevant set
of test files by analyzing the git repository's history.

Previously, the script referenced an environment variable provided by
TravisCI named `TRAVIS_COMMIT_RANGE` when running for GitHub.com pull
requests. That variable is documented [1] as follows:

> - `TRAVIS_COMMIT_RANGE`: The range of commits that were included in the push
>   or pull request. (Note that this is empty for builds triggered by the
>   initial commit of a new branch.)

Beyond being somewhat imprecise, that value has been found to be
incorrect in some cases [2]. The definition leaves open the possibility
for race conditions resulting from changes to the target branch over the
course of the TravisCI build life cycle.

Update the "changed file" detection heuristic to instead use the
`TRAVIS_BRANCH` variable, whose definition is much more precise and less
susceptible to race conditions of the type described above. In addition,
update the operator used to specify git endpoints in the subsequent
invocation of `git diff` as this syntax is distinct from that used to
specify ranges [3].

[1] https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
[2] travis-ci/travis-ci#7578
[3] https://git-scm.com/docs/git-diff#_description

>     git diff [--options] <commit>..<commit> [--] [<path>...]
>         This is synonymous to the previous form. If <commit> on one
>         side is omitted, it will have the same effect as using HEAD
>         instead.
>
>     git diff [--options] <commit>...<commit> [--] [<path>...]
>         This form is to view the changes on the branch containing and
>         up to the second <commit>, starting at a common ancestor of
>         both <commit>. "git diff A...B" is equivalent to "git diff
>         $(git-merge-base A B) B". You can omit any one of <commit>,
>         which has the same effect as using HEAD instead.
>
>     Just in case if you are doing something exotic, it should be noted
>     that all of the <commit> in the above description, except in the
>     last two forms that use ".." notations, can be any <tree>.
>
>     For a more complete list of ways to spell <commit>, see
>     "SPECIFYING REVISIONS" section in gitrevisions(7). However, "diff"
>     is about comparing two endpoints, not ranges, and the range
>     notations ("<commit>..<commit>" and "<commit>...<commit>") do not
>     mean a range as defined in the "SPECIFYING RANGES" section in
>     gitrevisions(7).
@ghost
Copy link

ghost commented Apr 6, 2017

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 9dec23c
Using browser at version BuildID 20170406100208; SourceStamp 3c68d659c2b715f811708f043a1e7169d77be2ba
Starting 10 test iterations
No tests run.

@ghost
Copy link

ghost commented Apr 6, 2017

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 9dec23c
Using browser at version 59.0.3063.4 dev
Starting 10 test iterations
No tests run.

@jgraham
Copy link
Contributor

jgraham commented Apr 7, 2017

Arghhhh. You are right about ....

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

Successfully merging this pull request may close these issues.

2 participants