chore(travis/build.sh): move ddescribe check before unit tests #13517

Narretz · 2015-12-11T17:30:13Z

This was changed as part of #9792.
While the logic is sound that style errors shouldn't block tests from
running, ddescribe should run before the tests.
Otherwise, when Travis exits with a warning after some browsers have run,
ddescribe doesn't get run and it doesn't immediately become apparent
that not all tests have run.

gkalpak · 2015-12-11T23:11:02Z

scripts/travis/build.sh

@@ -12,6 +12,7 @@ if [ $JOB = "unit" ]; then
    BROWSERS="SL_Chrome,SL_Safari,SL_Firefox,SL_IE_9,SL_IE_10,SL_IE_11,SL_iOS"
  fi

+  grunt ddescribe-iit


TBH, I would prefer running all ci-checks (except for jscs) first and just run jscs after the unit tests.

Narretz · 2015-12-14T21:57:41Z

I've created a new task that includes ddescribe-iit, jshint and merge-conflicts. jscs is run after the tests run.

gkalpak · 2015-12-15T11:56:24Z

Gruntfile.js

@@ -355,6 +355,7 @@ module.exports = function(grunt) {
  grunt.registerTask('minify', ['bower','clean', 'build', 'minall']);
  grunt.registerTask('webserver', ['connect:devserver']);
  grunt.registerTask('package', ['bower','clean', 'buildall', 'minall', 'collect-errors', 'docs', 'copy', 'write', 'compress']);
-  grunt.registerTask('ci-checks', ['ddescribe-iit', 'merge-conflict', 'jshint', 'jscs']);
+  grunt.registerTask('ci-pre-checks', ['ddescribe-iit', 'jshint', 'merge-conflict']);
+  grunt.registerTask('ci-checks', ['merge-conflict', 'jshint', 'jscs']);


Was removing ddescribe-iit from this task intended ?

Narretz · 2015-12-15T12:12:33Z

Updated. Good catches!

gkalpak · 2015-12-15T12:45:31Z

scripts/travis/build.sh

  grunt test:promises-aplus
  grunt test:unit --browsers $BROWSERS --reporters dots
-  grunt ci-checks
+  grunt jcsc


jcsc --> jscs (:smiley:)

gkalpak · 2015-12-15T12:46:13Z

LGTM (with typo fixed) 👍

petebacondarwin · 2015-12-15T13:34:28Z

I think it would be even better if we create a new job on travis that only runs the ci-checks.
This would mean that we could see if tests are passing even if the ci-checks are failing, while having a clear failure for the build as a whole if the ci-checks fail.

Narretz · 2015-12-15T13:42:47Z

Ok, sounds reasonable. The only downside I see is that it will increase the average build run time by a few minutes.

petebacondarwin · 2015-12-15T13:43:58Z

Not necessarily as they are run in parallel - also the ci-checks wouldn't use up tunnels as it is not doing any karma or protractor

gkalpak · 2015-12-15T13:51:44Z

If the ci-checks are failing it means that one of the following is happening:

ddescribe-iit fails - In that case, tests passing is not that informative.
merge-conflict fails - Most probably the source code is not parsable. Even if it (accidentally) is, there will need to be changes to remove the merge artifacts, so the tests might fails anyway.
jshint fails - There's a chance that the tests passing is informative, but not guarranteed.

So, I am not sure we gain much 😃 jscs is the only one that is not directly related to the tests passing//failing, but that is run at the end.

_{Just saying...}
_{(Either way is fine by me.)}

petebacondarwin · 2015-12-15T13:59:03Z

ci-checks also runs jscs.

The reason this is helpful is that when debugging it is helpful to send a commit with a ddescribe block to Travis to run just that block. But if the ddescribe test kills the build early you don't see if the test would have passed.

Narretz · 2015-12-15T14:00:04Z

When you are working on a fix, it's also nice to see at a glance if your tests failed, or if you just made a style error.

Narretz · 2015-12-15T14:31:39Z

I've created a new sh file which contains all the logic for the before_script block. However, I am getting this error for all jobs:

/home/travis/build.sh: line 45: ./scripts/travis/before_build.sh: Permission denied

The file isn't much different than the others. Aynone have an idea?

gkalpak · 2015-12-15T15:08:40Z

Could it be that directory you are trying to create ?

Narretz · 2015-12-15T16:48:04Z

The file was created witt the wrong permissions ... I've added the new job, PTAL

petebacondarwin · 2015-12-15T18:31:26Z

I was going to suggest a bit of chmod was needed :-)

gkalpak · 2015-12-15T18:40:58Z

LGTM

Previously, ddescribe, merge-conflicts, jshint, and jscs would run after unit & e2e tests ran. The order was orginally changed as part of angular#9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should always run. This was not guaraneteed; when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't become apparent that not all tests have run. Additionally, a separate job clearly separates style from test errors, which e.g. means you can open a PR that includes an iit to speed up the job, and see immediately if the test passes, because the ddescribe error is in another job.

petebacondarwin · 2015-12-16T10:16:50Z

Merging but tweaking the error message here: https://github.com/angular/angular.js/pull/13517/files#diff-014afcca28a4c58f73f0c7973192510cR35

petebacondarwin · 2015-12-16T10:22:39Z

Landed as 0b94e8a

Now checking that it works on 1.2, 1.3 and 1.4

petebacondarwin · 2015-12-16T10:41:04Z

Landed as

34e5623 on 1.2
7fa13c3 on 1.3
7882c1c on 1.4

petebacondarwin · 2015-12-16T10:41:09Z

Thanks @Narretz

Narretz added type: chore severity: confusing labels Dec 11, 2015

Narretz added this to the 1.5.0-rc.1 milestone Dec 11, 2015

googlebot added the cla: yes label Dec 11, 2015

gkalpak reviewed Dec 11, 2015
View reviewed changes

Narretz force-pushed the chore-travis-build branch 3 times, most recently from 1e021b3 to 20227e2 Compare December 14, 2015 21:57

gkalpak reviewed Dec 15, 2015
View reviewed changes

Narretz force-pushed the chore-travis-build branch from 20227e2 to 14b783b Compare December 15, 2015 12:10

gkalpak reviewed Dec 15, 2015
View reviewed changes

Narretz force-pushed the chore-travis-build branch from 14b783b to 241ce8c Compare December 15, 2015 13:09

Narretz force-pushed the chore-travis-build branch 2 times, most recently from 04f019b to aadcf01 Compare December 15, 2015 14:23

Narretz force-pushed the chore-travis-build branch from aadcf01 to eeb9b3b Compare December 15, 2015 14:34

Narretz force-pushed the chore-travis-build branch 2 times, most recently from 1344b85 to b22f0f0 Compare December 15, 2015 15:48

Narretz force-pushed the chore-travis-build branch 7 times, most recently from 57f04c9 to 010eda8 Compare December 15, 2015 16:20

Narretz force-pushed the chore-travis-build branch from 010eda8 to b716828 Compare December 15, 2015 22:12

petebacondarwin closed this Dec 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(travis/build.sh): move ddescribe check before unit tests #13517

chore(travis/build.sh): move ddescribe check before unit tests #13517

Narretz commented Dec 11, 2015

gkalpak Dec 11, 2015

Narretz commented Dec 14, 2015

gkalpak Dec 15, 2015

Narretz commented Dec 15, 2015

gkalpak Dec 15, 2015

gkalpak commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

Narretz commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

gkalpak commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

Narretz commented Dec 15, 2015

Narretz commented Dec 15, 2015

gkalpak commented Dec 15, 2015

Narretz commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

gkalpak commented Dec 15, 2015

petebacondarwin commented Dec 16, 2015

petebacondarwin commented Dec 16, 2015

petebacondarwin commented Dec 16, 2015

petebacondarwin commented Dec 16, 2015

chore(travis/build.sh): move ddescribe check before unit tests #13517

chore(travis/build.sh): move ddescribe check before unit tests #13517

Conversation

Narretz commented Dec 11, 2015

gkalpak Dec 11, 2015

Choose a reason for hiding this comment

Narretz commented Dec 14, 2015

gkalpak Dec 15, 2015

Choose a reason for hiding this comment

Narretz commented Dec 15, 2015

gkalpak Dec 15, 2015

Choose a reason for hiding this comment

gkalpak commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

Narretz commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

gkalpak commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

Narretz commented Dec 15, 2015

Narretz commented Dec 15, 2015

gkalpak commented Dec 15, 2015

Narretz commented Dec 15, 2015

petebacondarwin commented Dec 15, 2015

gkalpak commented Dec 15, 2015

petebacondarwin commented Dec 16, 2015

petebacondarwin commented Dec 16, 2015

petebacondarwin commented Dec 16, 2015

petebacondarwin commented Dec 16, 2015