Update unit test dependencies #2068

benjaminapetersen · 2017-09-13T19:26:56Z

Updates karma related dependencies for unit testing.

Looking into why a few tests now fail, will update again shortly (did we always have this pretty coverage tool?)(is a lot of red reported by that pretty coverage tool 😄 ).

to fix:

one of the updates, probably jasmine, is more strictly comparing objects than previously, resulting in some tests failing.
the coverage table is being output twice since I'm running the tests in Firefox & Chrome concurrently by default. This is a little annoying.

@spadgett

benjaminapetersen · 2017-09-13T19:32:16Z

I these tests are failing because they are a little sloppy, basically json1 === json2. There are new properties added to the objects. I'll try to make them specific & fix.

spadgett · 2017-09-13T19:32:25Z

@benjaminapetersen Awesome.

The errors are because

{foo: undefined}

is not equal to

{}

We're explicitly setting some fields to undefined in the services.

spadgett · 2017-09-13T20:26:02Z

@benjaminapetersen Have a look at

spadgett@f27b948#diff-33e8a7d478aa060b7e40588f2ebd63ec

and

spadgett@f27b948#diff-ddf9d4dc92739022396ec6d3f59795e9

We might need to update Travis to use Node 6.x as well.

spadgett@f27b948#diff-354f30a63fb0907d4ad57269548329e3

benjaminapetersen · 2017-09-14T03:14:01Z

@spadgett thanks! I didn't see your comment until my last push, but let me know your thoughts on this. I updated the tests (where they now fail) to break them down into very specific parts. If there is a failure, its focused on a single value or an object of much smaller complexity. Pros and cons:

much easier to diagnose a failure & make a change
harder to see what the overall structure is supposed to be

I think the pros outweigh the cons since I always end up running the functions and console.log(JSON.stringify(someMethod.getBigBlobOutput())) anyway.

Another potential con, I got around the sudden missing namespace property causing the failures by ignoring the property. This is potentially bad. Is this the only case where a new property might get added? Is it appropriate to rely on tests to catch that, or should we update the tests by adding another test for the property if the property is important.

Def open to your thoughts & further adjustments.
Protractor (e2e) updates coming in a separate PR.

benjaminapetersen · 2017-09-14T03:14:10Z

[test]

benjaminapetersen · 2017-09-14T03:44:19Z

Ooop, forgot to drop Chrome from the list.

[test]

spadgett · 2017-09-14T12:52:55Z

@benjaminapetersen I agree it makes it easier to diagnose failures, but I worry the tests are now passing with "bad" data. namespace: undefined should not be there. If we don't check the complete objects, we'll let things we're not checking for through. Another common problem would be not stripping $$foo angular values from the objects before we send them to the API. If we don't test the full object, we'll miss stuff like that.

I'd rather fix the services not to set undefined values in the objects. That to me seems like the bug.

There are also things like

https://www.npmjs.com/package/karma-jasmine-diff-reporter

to better see what the diff is. That seems cleaner to me than rewriting our tests.

benjaminapetersen · 2017-09-14T18:00:36Z

Agreed, I'll try that diff reporter & update this. Curious if there is a good argument for both, the more explict test next to the blob comparison.

benjaminapetersen · 2017-09-14T18:09:07Z

Opened an issue with that reporter, the installation instructions are either incomplete or something is broken. reporter issue

benjaminapetersen · 2017-09-14T18:38:13Z

Ok, since that diff reporter doesn't work, I kept both versions of the test. Hopefully will be short term, but I really dislike debugging these as blob vs blob. Will try to make progress on that issue as a follow-up.

Meanwhile, this will get unit test dependencies 100% current, would like to get back to protractor for e2e & get that working on Mac.

[test]

spadgett · 2017-09-14T18:55:20Z

test/spec/services/applicationGeneratorSpec.js

-      expect(resources.buildConfig).toEqual(
+      var generatedBuildConfig = resources.buildConfig;
+
+      // specific tests to ease pain in debugging


In the long run, these are going to make the tests harder to maintain. We've repeated everything twice, and it's a lot of work to write these. I'm not sure it's worth the tradeoff.

@benjaminapetersen I'd rather not test the same things twice. Since we have the diff reporter, let's remove this.

The diff reporter states this is legacy behavior. Boo. I'm working on getting more info from the developer. It may mean that he isn't supporting it anymore. Mind if I hold off on deleting the code till I have a good answer? I hate to update all our dependencies just to end up with a deprecated dependency that will fall over on us again. Hopefully will get an answer soon.

benjaminapetersen · 2017-09-15T01:35:17Z

flake?

+ ssh -F ./.config/origin-ci-tool/inventory/.ssh_config -t openshiftdevel 'bash -l -c "timeout 300 /tmp/tmp.jjTPwTJqTB"'
+ cd /data/src/github.com/openshift/aos-cd-jobs
+ trap 'exit 0' EXIT
+ sjb/gcs/started.py
++ exit 0
++ export status=FAILURE
++ status=FAILURE
+ set +o xtrace
########## FINISHED STAGE: FAILURE: RECORD THE STARTING METADATA [00h 05m 00s] ##########
Build step 'Execute shell' marked build as failure
[PostBuildScript] - Execution post build scripts.
[workspace] $ /bin/bash /tmp/jenkins8090829258080536932.sh

[test]

benjaminapetersen · 2017-09-15T02:23:47Z

Fail cuz Firefox:

Running "karma:unit" (karma) task
14 09 2017 21:54:48.085:ERROR [launcher]: Cannot start Firefox
	
14 09 2017 21:54:48.087:ERROR [launcher]: Firefox stdout: 
14 09 2017 21:54:48.087:ERROR [launcher]: Firefox stderr: 
14 09 2017 21:54:48.088:ERROR [launcher]: Firefox failed 2 times (cannot start). Giving up.
Warning: Task "karma:unit" failed.� Use --force to continue.

Aborted due to warnings.


Execution Time (2017-09-15 01:54:25 UTC)
loading tasks    364ms  ▇ 2%
concurrent:test   5.4s  ▇▇▇▇▇▇▇▇▇▇▇ 24%
postcss:dist     677ms  ▇▇ 3%
karma:unit       16.3s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 71%
Total 22.8s

make: *** [test] Error 3
++ export status=FAILURE
++ status=FAILURE
+ set +o xtrace
########## FINISHED STAGE: FAILURE: RUN WEB CONSOLE TESTS [00h 06m 46s] ##########

Good grief. Specified older FF in Travis similar to catalog 432.

[test], plz.

benjaminapetersen · 2017-09-15T03:00:53Z

Travis does not appear to honor the browsers flag in our .travis.yml:

language: node_js
node_js:
  - "6"
before_script:
  - make build
script:
  - grunt test --browsers=PhantomJS # hmmm.
  - hack/verify-dist.sh
addons:
  firefox "49.0"

benjaminapetersen · 2017-09-15T03:17:02Z

Trying Nightmare because Firefox is being that anyway...

[test]

spadgett · 2017-09-15T11:42:09Z

Good grief. Specified older FF in Travis similar to catalog 432.

@benjaminapetersen I think you need a virtual screen. Let's try with current Firefox.

before_script:
  - export DISPLAY=:99.0
  - sh -e /etc/init.d/xvfb start

benjaminapetersen · 2017-09-15T14:18:20Z

Figured out the config tweak to get the diff reporter to work:

Requires "legacy: true" flag because Jasmine has its own diff now, but the built-in diff I'm seeing is not awesome. Fishing around to see if we need to worry about legacy mode.

benjaminapetersen · 2017-09-15T14:19:29Z

@spadgett I'll add that, though its odd that unit testing would require a window. Lets see.

benjaminapetersen · 2017-09-15T14:40:33Z

@spadgett We had caching of node modules between updates didn't we? Seems like testing is taking longer again.

spadgett · 2017-09-15T14:44:11Z

Running "karma:unit" (karma) task
15 09 2017 10:42:43.235:ERROR [launcher]: Cannot start Nightmare
	
15 09 2017 10:42:43.237:ERROR [launcher]: Nightmare stdout: 
15 09 2017 10:42:43.237:ERROR [launcher]: Nightmare stderr: 
15 09 2017 10:42:43.416:ERROR [launcher]: Cannot start Nightmare
	
15 09 2017 10:42:43.416:ERROR [launcher]: Nightmare stdout: 
15 09 2017 10:42:43.416:ERROR [launcher]: Nightmare stderr: 
15 09 2017 10:42:43.592:ERROR [launcher]: Cannot start Nightmare
	
15 09 2017 10:42:43.593:ERROR [launcher]: Nightmare stdout: 
15 09 2017 10:42:43.593:ERROR [launcher]: Nightmare stderr: 
15 09 2017 10:42:43.593:ERROR [launcher]: Nightmare failed 2 times (cannot start). Giving up.
Warning: Task "karma:unit" failed.� Use --force to continue.

spadgett · 2017-09-15T14:45:51Z

@benjaminapetersen We don't cache dependencies anymore, but it doesn't make that big of a difference. Most of the time is spent elsewhere.

Test job usually takes about 20-30 minutes.

benjaminapetersen · 2017-09-15T14:53:34Z

Swapping back Firefox for Nightmare. Works fine locally, should use FF for Travis.

spadgett · 2017-09-15T15:16:16Z

Looks like Jenkins has the same Firefox error as Travis

benjaminapetersen · 2017-09-15T16:00:07Z

Swapping to ['PhantomJS'] from ['Firefox'] temporarily just to see if we can get something to run.

@stevekuznetsov I may request some assistance from you diagnosing my Firefox dilemma if this doesn't work.

benjaminapetersen · 2017-09-15T16:29:59Z

Adding a sleep 3 to .travis.yml to give xvfb time to start. maybe timing issue? Is what we did originally with key-value-editor to get it working.

spadgett · 2017-09-15T16:31:22Z

Looks like unit tests ran in Jenkins, but later starting protractor we get this:

Error: No selenium server jar found at the specified location (/data/src/github.com/openshift/origin-web-console/node_modules/grunt-protractor-runner/node_modules/protractor/selenium/selenium-server-standalone-2.45.0.jar). Check that the version number is up to date.

spadgett · 2017-09-15T16:37:06Z

package.json

-    "protractor": "1.7.0",
-    "protractor-screenshot-reporter": "0.0.5",
+
+    "protractor": "^5.1.2",


Did you mean to bump protractor in this pull? I thought other changes were needed for that.

Ah, crap, my last swap between branches must have accidentally committed that, thanks. Will update and see where that gets us.

spadgett · 2017-09-15T16:38:37Z

package.json

@@ -69,6 +84,7 @@
    "serve": "grunt serve",
    "start": "grunt serve",
    "test-integration": "grunt test-integration",
-    "postinstall": "node test/upgrade-selenium.js && node_modules/protractor/bin/webdriver-manager update"
+    "__postinstall": "node test/upgrade-selenium.js && node_modules/protractor/bin/webdriver-manager update",
+    "_postinstall": "node_modules/protractor/bin/webdriver-manager update"


This might be causing the Jenkins failure?

same as above, putting this back.

benjaminapetersen · 2017-09-15T17:50:48Z

Ok put the protractor stuff back, lets see if this test runs.

spadgett · 2017-09-15T18:22:24Z

FINISHED STAGE: FAILURE: PROVISION CLOUD RESOURCES [00h 10m 20s]

[test]

benjaminapetersen · 2017-09-15T18:51:01Z

Yay Jenkins survived this time. Now for Travis...

spadgett · 2017-09-15T19:11:22Z

Looks like the previous Travis run worked, but now it's stuck.

spadgett · 2017-09-15T19:11:46Z

.travis.yml

+  # even our unit tests require a
+  - export DISPLAY=:99.0
+  - sh -e /etc/init.d/xvfb start
+  - sleep 3 # give xvfb some time to start


We should be able to remove this if we're using PhantomJS (for now anyway)

I will comment out all of this display stuff & leave another FIXME (& note it in the issue)

I will comment out all of this display stuff & leave another FIXME (& note it in the issue)

I'd rather just remove it to keep the code tidy :)

We could add this to the issue if necessary.

spadgett · 2017-09-15T19:12:34Z

Gruntfile.js

+                    // if running locally on mac, we can test both FF & Chrome,
+                    // in Travis, just FF
+                    // ['Nightmare'] is a good alt for a current headless
+                    // TODO: fix this, PhantomJS is deprecated


Let's track as an issue instead of a TODO in the code

ok ill update to FIXME & open an issue.

I'd remove the comment altogether and just use the issue. I'd prefer to track things once, and TODOs tend to get forgotten

spadgett · 2017-09-15T19:13:42Z

test/karma.conf.js

    ],

    // Continuous Integration mode
    // if true, it capture browsers, run tests and exit
-    singleRun: false,
-
-    colors: true,


Do we want to try and add colors back?

yup can do.

spadgett · 2017-09-15T19:14:44Z

test/karma.conf.js


    // level of logging
    // possible values: LOG_DISABLE || LOG_ERROR || LOG_WARN || LOG_INFO || LOG_DEBUG
-    logLevel: config.LOG_DEBUG,
+    logLevel: config.LOG_ERROR,


Any specific reason to change this from debug?

It was just noisy, was trying to cut down on stuff to swim through. Let me roll that back.

benjaminapetersen · 2017-09-15T19:25:19Z

Ok, so we had one successful run? Pushing updates per the comments &...

[test]

again.

benjaminapetersen · 2017-09-15T21:08:37Z

Travis hangs again. I sad. ☹️

[test]

spadgett

LGTM, but please squash. Thanks @benjaminapetersen, happy to see these updated

benjaminapetersen · 2017-09-16T00:05:58Z

OOoooo it worked. neato. Will squash soon & be glad to be ready to roll with this.

stevekuznetsov · 2017-09-16T00:46:03Z

Looks like it may have been in the Travis queue for a while there.

benjaminapetersen · 2017-09-16T01:21:21Z

Squashed.

[test]

Btw, I had to swap back logLevel: config.LOG_ERROR, from LOG_DEBUG as there is so much noise from all the file loading that it causes the tests to flake at times (locally, and it overwhelms the meaningful test output).

spadgett · 2017-09-16T10:46:54Z

Origin Web Console Test Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/214/) (Base Commit: 5bf079c) (PR Branch Commit: 91fe691)

flake #1685

[test]

openshift-bot · 2017-09-16T10:47:21Z

Evaluated for origin web console test up to 91fe691

openshift-bot · 2017-09-16T11:10:33Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/216/) (Base Commit: 5bf079c) (PR Branch Commit: 91fe691)

spadgett · 2017-09-16T13:52:36Z

[merge]

openshift-bot · 2017-09-16T13:55:19Z

Evaluated for origin web console merge up to 91fe691

openshift-bot · 2017-09-16T13:55:28Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/205/) (Base Commit: 1466f66) (PR Branch Commit: 91fe691)

benjaminapetersen requested a review from spadgett September 14, 2017 03:15

benjaminapetersen force-pushed the bpetersen/unit-test-dependency-udpates branch from 6f81b61 to b99d376 Compare September 14, 2017 03:44

benjaminapetersen mentioned this pull request Sep 14, 2017

Unit tests need a good output diff tool #2079

Closed

benjaminapetersen force-pushed the bpetersen/unit-test-dependency-udpates branch from b99d376 to dd372aa Compare September 14, 2017 18:36

spadgett reviewed Sep 14, 2017

View reviewed changes

benjaminapetersen mentioned this pull request Sep 15, 2017

Update e2e test dependency updates & get them running on Mac #2083

Closed

7 tasks

spadgett reviewed Sep 15, 2017

View reviewed changes

benjaminapetersen mentioned this pull request Sep 15, 2017

Tests should use Firefox instead of PhantomJS in CI #2091

Closed

2 tasks

spadgett reviewed Sep 15, 2017

View reviewed changes

benjaminapetersen force-pushed the bpetersen/unit-test-dependency-udpates branch from 3eb08a0 to 9d1c673 Compare September 16, 2017 01:19

Update unit test dependencies

91fe691

benjaminapetersen force-pushed the bpetersen/unit-test-dependency-udpates branch from 9d1c673 to 91fe691 Compare September 16, 2017 01:21

spadgett approved these changes Sep 16, 2017

View reviewed changes

openshift-bot merged commit 1b695d8 into openshift:master Sep 16, 2017

benjaminapetersen deleted the bpetersen/unit-test-dependency-udpates branch September 16, 2017 20:32

spadgett mentioned this pull request Sep 26, 2017

Bug 1495460 - Fix build config source namespace #2169

Merged

Update unit test dependencies #2068

Update unit test dependencies #2068

Conversation

benjaminapetersen commented Sep 13, 2017 • edited by spadgett Loading

benjaminapetersen commented Sep 13, 2017

spadgett commented Sep 13, 2017 • edited Loading

spadgett commented Sep 13, 2017

benjaminapetersen commented Sep 14, 2017

benjaminapetersen commented Sep 14, 2017

benjaminapetersen commented Sep 14, 2017

spadgett commented Sep 14, 2017

benjaminapetersen commented Sep 14, 2017

benjaminapetersen commented Sep 14, 2017

benjaminapetersen commented Sep 14, 2017

Choose a reason for hiding this comment

spadgett Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

spadgett commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

spadgett commented Sep 15, 2017

spadgett commented Sep 15, 2017 • edited Loading

benjaminapetersen commented Sep 15, 2017

spadgett commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

spadgett commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Sep 15, 2017

spadgett commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

spadgett commented Sep 15, 2017

Choose a reason for hiding this comment

benjaminapetersen Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Sep 15, 2017

benjaminapetersen commented Sep 15, 2017

spadgett left a comment

Choose a reason for hiding this comment

benjaminapetersen commented Sep 16, 2017

stevekuznetsov commented Sep 16, 2017

benjaminapetersen commented Sep 16, 2017 • edited Loading

spadgett commented Sep 16, 2017

openshift-bot commented Sep 16, 2017

openshift-bot commented Sep 16, 2017

spadgett commented Sep 16, 2017

openshift-bot commented Sep 16, 2017

openshift-bot commented Sep 16, 2017 • edited Loading

benjaminapetersen commented Sep 13, 2017 •

edited by spadgett

Loading

spadgett commented Sep 13, 2017 •

edited

Loading

spadgett Sep 15, 2017 •

edited

Loading

spadgett commented Sep 15, 2017 •

edited

Loading

benjaminapetersen Sep 15, 2017 •

edited

Loading

benjaminapetersen commented Sep 16, 2017 •

edited

Loading

openshift-bot commented Sep 16, 2017 •

edited

Loading