build(ci): skip tests on CI when only markdown files have changed #4452

chouclee · 2017-05-10T00:15:47Z

Skip both unit tests and e2e tests on CI when commits contain only markdown files (*.md)
solves #3967

devversion · 2017-05-19T22:08:28Z

scripts/ci/build-and-test.sh

+for filename in $(git diff --name-only $TRAVIS_BRANCH...HEAD); do
+  if ! [[ "$filename" =~ .*\.md ]]; then
+    skip_tests=false
+    break


Should we check the modes here and just exit before?

Good point. Let me try to change it.

devversion · 2017-05-19T22:09:25Z

scripts/ci/build-and-test.sh

@@ -10,20 +10,29 @@ cd $(dirname $0)/../..
 source scripts/ci/sources/mode.sh
 source scripts/ci/sources/tunnel.sh

+# Check if tests can be skipped
+skip_tests=true
+for filename in $(git diff --name-only $TRAVIS_BRANCH...HEAD); do


Would it make more sense to use TRAVIS_COMMIT_RANGE?

@devversion
Please check: travis-ci/travis-ci#2668 TRAVIS_COMMIT_RANGE will include outdated commit if user does a force push (which is very common to update a PR)

But If I recall correctly, for Push builds the $TRAVIS_BRANCH will be master and the merged commit will be HEAD / master as well.

This would mean that the diff will be always empty for push builds.

@devversion HEAD would be PR merged into master when $TRAVIS_BRANCH is set to master. Then the diff would be your PR commits. You can try it.

I already tested this PR within my forked material2 repo. And just did another one:
https://travis-ci.org/chouclee/material2/builds/234685761 (md file change only)
https://travis-ci.org/chouclee/material2/builds/234687221 (md and js file, this is a hard push to update PR).

You can find the tests were skipped in first CI build, but not in second one.

I didn't talk about PRs. Also good that you tested it for PRs.

Can you try merging your PR on your fork and see if everything works there too?

I see what you meant. You are right, for Push build, the diff would be empty.
I should check the $TRAVIS_PULL_REQUEST first.

And I did a little search:
https://github.com/GoogleCloudPlatform/python-docs-samples/blob/72fa14b3957cacfdfe3113d14b8d77204f7ec9f7/nox.py#L84

If it's a Push, there are two choices. $TRAVIS_COMMIT or $TRAVIS_COMMIT_RANGE. Then the question is, do material2 allow any user directly push commits to master without a PR? If it's not allowed, then both variables look fine to me since we cannot force update a Pull Request commit.

chouclee · 2017-05-20T00:25:24Z

Another thing came to my mind, maybe we can just skip CI build if only docs are being changed.
What do you think about? @jelbourn

jelbourn · 2017-05-20T00:32:25Z

@chouclee isn't that what this already is?

devversion · 2017-05-20T10:21:14Z

@chouclee You are talking about the "per-commit" builds right?

I don't think we should skip those. The material2-docs-content still needs to be deployed and I think it's still good to publish the material2-builds for every commit that lands in master. (As same as in @angular/angular)

chouclee · 2017-05-22T08:27:07Z

@devversion PR updated.

devversion · 2017-05-22T14:04:44Z

scripts/ci/build-and-test.sh

+    skip_tests=false
+    break
+  fi
+done


Thinking if we could make it more simple

if [[ "$TRAVIS_PULL_REQUEST" = "false" ]]; then fileDiff=$(git diff --name-only $TRAVIS_COMMIT_RANGE) else fileDiff=$(git diff --name-only $TRAVIS_BRANCH...HEAD) fi if [[ ${fileDiff} =~ ^(.*\.md\s*)*$ ]] && (is_e2e || is_unit); then echo "Skipping e2e and unit tests since only markdown files changed" exit 0 fi

Cool, I just learned something new from you.

Only two things:

[[ "$TRAVIS_PULL_REQUEST" = "false" ]] would print as +[[ false = \f\a\l\s\e ]] in Travis output, and quotes are unnecessary.
[ "$TRAVIS_PULL_REQUEST" = "false" ] would print as ``+[[ false = false ]]. I think it's more readable.

I changed the regex ${fileDiff} =~ ^(.*\.md\s*)*$ to ${fileDiff} =~ ^(.*\.md\s*)+$ . Although I doubt anyone would push an empty commit...

No problem

I didn't write that. Just copied it from your PR changes

It's fine. But I prefer the * just to be on the saf eside.

Do you mind sharing that why * is safer here?

Just in case there would be any empty commit. Which happens very unlikely but it just gives us the flexibility. No real benefit of using +

I am confused... + is to tackle the empty commit case (e.g. trigger CI with an empty commit)...
If it's an empty commit, * would skip the tests, but + would't. The feature here is to skip tests only md files are changed. That's why I use + -- skip tests iff md files are changed.

If the commit is empty then it's fine to skip the tests as well. No source code change is 100% guaranteed.

Sure. I restored + to *.

Skip e2e and unit tests for Push/PR build

devversion

LGTM

jelbourn

LGTM

devversion · 2017-05-25T20:48:52Z

@chouclee By the way. Thanks for your work! It works perfectly 🎉

chouclee · 2017-05-30T04:25:12Z

@devversion I am glad to hear that. Thanks for your review. 😄

angular-automatic-lock-bot · 2019-09-06T13:02:00Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 10, 2017

chouclee changed the title ~~build(ci): skip tests if only markdown files are changed~~ build(ci): skip tests on CI when only markdown files have changed May 10, 2017

chouclee closed this May 10, 2017

chouclee reopened this May 10, 2017

jelbourn requested a review from devversion May 15, 2017 22:00

jelbourn assigned devversion May 15, 2017

jelbourn added the pr: needs review label May 15, 2017

devversion reviewed May 19, 2017

View reviewed changes

chouclee closed this May 22, 2017

chouclee deleted the build-skip-tests branch May 22, 2017 04:38

chouclee restored the build-skip-tests branch May 22, 2017 04:38

chouclee reopened this May 22, 2017

chouclee closed this May 22, 2017

chouclee reopened this May 22, 2017

devversion reviewed May 22, 2017

View reviewed changes

build(ci): skip tests on CI when only markdown files have changed

2bf5533

Skip e2e and unit tests for Push/PR build

devversion approved these changes May 23, 2017

View reviewed changes

devversion added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 23, 2017

jelbourn approved these changes May 23, 2017

View reviewed changes

jelbourn added the pr: merge safe label May 23, 2017

tinayuangao merged commit aee42ae into angular:master May 23, 2017

chouclee deleted the build-skip-tests branch May 23, 2017 21:17

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

build(ci): skip tests on CI when only markdown files have changed #4452

build(ci): skip tests on CI when only markdown files have changed #4452

Uh oh!

Conversation

chouclee commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chouclee May 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chouclee May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chouclee May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chouclee commented May 20, 2017

Uh oh!

jelbourn commented May 20, 2017

Uh oh!

devversion commented May 20, 2017

Uh oh!

chouclee commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chouclee May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chouclee May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

devversion commented May 25, 2017

Uh oh!

chouclee commented May 30, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 6, 2019

Uh oh!

Uh oh!

chouclee commented May 10, 2017 •

edited

Loading

chouclee May 20, 2017 •

edited

Loading

chouclee May 22, 2017 •

edited

Loading

devversion May 22, 2017 •

edited

Loading

chouclee May 22, 2017 •

edited

Loading

chouclee May 22, 2017 •

edited

Loading

chouclee May 22, 2017 •

edited

Loading