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

deps: upgrade #218

Merged
merged 21 commits into from Jan 23, 2019

Conversation

Projects
None yet
2 participants
@adampash
Copy link
Contributor

adampash commented Jan 18, 2019

This PR upgrades as many dependencies as possible. The dependencies that were not upgraded were either forks or items that could not be updated without a lot of rework.

The main Mercury dependency (that is, not a devDependency) that did not get a major upgrade and was not a fork is cheerio. The app depends on it heavily, and the upgrade sets a lot of tests failing without a quick/obvious fix. It would be nice to upgrade cheerio eventually, but now isn't the best time.

@adampash adampash requested a review from george-haddad Jan 18, 2019

@george-haddad

This comment has been minimized.

Copy link
Collaborator

george-haddad commented Jan 21, 2019

Sanity checking the following 4 commands has led me to raise 6 issues

yarn build # build node release
yarn build:web # build web release
yarn test:node # run tests on node
yarn test:web # run tests on web

1 - Mac/Linux specific script in lint script

The following lint script

"lint": "if test \"$CI\" != \"true\" ; then eslint . --fix; fi",

is tightly coupled to 2 platforms (Mac/Linux) and will fail on Windows. Also I tried looking around to see where this is actually needed. I noticed that in the CircleCi config it is directly calling lint:ci so I'm wondering if the other condition will ever get executed? Either way, changing it to "lint": "eslint . --fix", gets everything to work on Windows.

2 - Security warnings for node-uuid package

While running yarn build there is the following security warning, it seems to continue without it, though I'm sure someone will bring it up and something will have to do something about it.

Warning:  [SECURITY] node-uuid: crypto not usable, falling back to insecure Math.random()

3 - Increase timeout on test in check-build.test.js

it(`gets this title right ${article.title}`, done => {
Merc.parse(article.url)
.then(result => {
assert.equal(article.title, result.title);
done();
})
.catch(e => {
console.log(e.name, e.message); // eslint-disable-line no-console
assert.equal(true, false);
done();
});
})

The above it code block will run using the default 5s timeout. On my machine and where I am in the world and my internet connection speed ...etc, that test finishes in 6 or 7 seconds. So adding a 10 second timeout to it stops it from intermittently failing. I also recommend that all tests that require connecting to a URL be given a 10 second custom timeout just to avoid this issue.

4 - Prepend node command to test:web

Running yarn build:web will fail on Windows for 2 specific reasons.

  1. Need to prepend node to this line
    "test:web": "./node_modules/karma/bin/karma start karma.conf.js --auto-watch",
  2. Need to also prepend node to
    require('child_process').execSync(
    './node_modules/karma/bin/karma start ./scripts/karma.conf.js',
    { stdio: [0, 1, 2] }
    );

Point no.2 raises a question for me, why it this command run inside a script and is it possible to run in package.json just like "test:web" script?

"test:web": "./node_modules/karma/bin/karma start karma.conf.js --auto-watch",

Could this be moved over to package.json? but if not prepending node to the command makes it work on windows or else an error will be thrown complaining that ./ is not recognized.

5 - Tests failing due to default timeout being exceeded

Running yarn build:web will yield the following errors

21 01 2019 17:41:43.969:INFO [framework.browserify]: bundle built
21 01 2019 17:41:43.995:INFO [karma-server]: Karma v3.1.4 server started at http://0.0.0.0:9876/
21 01 2019 17:41:43.995:INFO [launcher]: Launching browsers Chrome with concurrency unlimited
21 01 2019 17:41:44.015:INFO [launcher]: Starting browser Chrome
21 01 2019 17:41:46.178:INFO [Chrome 71.0.3578 (Windows 10.0.0)]: Connected on socket b-rUeaZ1PyzJt8l_AAAA with id 71254062
LOG: 'Error', 'CORS request rejected: http://www.cnn.com/2016/11/05/middleeast/iraq-mosul-isis-offensive/'
Chrome 71.0.3578 (Windows 10.0.0) Is Mercury build working gets this title right Iraqi troops storm town south of Mosul FAILED
        Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
LOG: 'Error', 'CORS request rejected: https://www.cnn.com/2018/10/12/us/before-after-aerial-images-mexico-beach-devastation-trnd/index.html'
Chrome 71.0.3578 (Windows 10.0.0) Is Mercury build working gets this title right Before and after images show there's nothing left in some parts of Mexico Beach FAILED
        Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
Chrome 71.0.3578 (Windows 10.0.0): Executed 2 of 2 (2 FAILED) ERROR (20.005 secs / 20.004 secs)
child_process.js:656
    throw err;
    ^

Error: Command failed: node ./node_modules/karma/bin/karma start ./scripts/karma.conf.js
    at checkExecSyncError (child_process.js:616:11)
    at Object.execSync (child_process.js:653:13)
    at Object.<anonymous> (D:\git\postlight\mercury\mercury-parser\scripts\proxy-browser-test.js:36:28)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:283:19)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I feel it's similar to point no.3 that adding a 10s timeout to the tests will resolve this. I couldn't find where to do this off the top of my head as it's the first time I go into the code base, so if you also analyze this and reach the same conclusion, I'd love to know where we can set this timeout option

6 - Two failed tests when running test:node

Running yarn test:node results in 2 failed tests

Summary of all failing tests
 FAIL  src/extractors/custom/www.msn.com/index.test.js
  ● MSNExtractor › initial test case › returns the date_published

    AssertionError [ERR_ASSERTION]: '2016-09-20' == '2016-09-21'

      61 |       // Update these values with the expected values from
      62 |       // the article.
    > 63 |       assert.equal(date_published.split('T')[0], '2016-09-21');
         |              ^
      64 |     });
      65 |
      66 |     it('returns the lead_image_url', async () => {

      at Object.equal (src/extractors/custom/www.msn.com/index.test.js:63:14)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (src/extractors/custom/www.msn.com/index.test.js:17:103)
      at _next (src/extractors/custom/www.msn.com/index.test.js:19:194)

 FAIL  src/extractors/generic/index.test.js
  ● GenericExtractor › extract(opts) › extracts this old LA Times article

    assert.equal(received, expected) or assert(received)

    Expected value to be equal to:
      "2009-10-14"
    Received:
      "2009-10-13"

    Difference:

    - Expected
    + Received

    - 2009-10-14
    + 2009-10-13

      21 |         'California appears poised to be first to ban power-guzzling big-screen TVs'
      22 |       );
    > 23 |       assert.equal(date_published.split('T')[0], '2009-10-14');
         |              ^
      24 |       assert.equal(dek, null);
      25 |     });
      26 |

      at Object.equal (src/extractors/generic/index.test.js:23:14)


Test Suites: 2 failed, 175 passed, 177 total
Tests:       2 failed, 863 passed, 865 total
Snapshots:   0 total
Time:        42.91s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I'm not 100% sure about this but if these tests are passing on your end but not on mine and it has to do with checking dates, it could be that our systems are configured with different time zones which is affecting the dates and causing this discrepancy in the results.

Same kind of failure for running yarn test:web

$ node ./node_modules/karma/bin/karma start karma.conf.js --auto-watch
21 01 2019 17:44:49.974:INFO [framework.browserify]: bundle built
21 01 2019 17:44:50.083:INFO [karma-server]: Karma v3.1.4 server started at http://0.0.0.0:9876/
21 01 2019 17:44:50.084:INFO [launcher]: Launching browsers Chrome with concurrency unlimited
21 01 2019 17:44:50.104:INFO [launcher]: Starting browser Chrome
21 01 2019 17:44:51.928:INFO [Chrome 71.0.3578 (Windows 10.0.0)]: Connected on socket 1NDpTIbM5DDxVTthAAAA with id 34954879
Chrome 71.0.3578 (Windows 10.0.0): Executed 605 of 860 SUCCESS (0 secs / 0.53 secs)
21 01 2019 17:44:54.553:WARN [web-server]: 404: /images/pixel.gif
Chrome 71.0.3578 (Windows 10.0.0): Executed 658 of 860 SUCCESS (0 secs / 0.868 secs)
21 01 2019 17:44:54.827:WARN [web-server]: 404: /images/pixel.gif
Chrome 71.0.3578 (Windows 10.0.0) GenericExtractor extract(opts) extracts this old LA Times article FAILED
        AssertionError: '2009-10-13' == '2009-10-14'
            at Object.<anonymous> (c:/Users/GEORGE~1/AppData/Local/Temp/src/extractors/generic/index.test.js:23:6 <- C:/Users/GEORGE~1/AppData/Local/Temp/419a6d20623bf43dbb7d62f5ae5b859f.browserify.js:95537:23)
Chrome 71.0.3578 (Windows 10.0.0): Executed 667 of 860 (1 FAILED) (0 secs / 0.925 secs)
21 01 2019 17:44:55.297:WARN [web-server]: 404: /sprite/etc/foo.png
Chrome 71.0.3578 (Windows 10.0.0): Executed 860 of 860 (1 FAILED) (2.372 secs / 1.388 secs)
TOTAL: 1 FAILED, 859 SUCCESS
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
@adampash

This comment has been minimized.

Copy link
Contributor Author

adampash commented Jan 22, 2019

I responded to each issue individually below, but the tl;dr here is: If it's an environment issue (Windows vs. *nix or timezone-related), let's pull that out into a separate task/PR so we can get this merged asap. Mostly I just want to make sure that the deps are working so I don't get in the way of other PRs getting merged. @pascalwatwat can you work with @george-haddad to extract the necessary pieces from this into separate task(s)?

Also I tried looking around to see where this is actually needed. I noticed that in the CircleCi config it is directly calling lint:ci so I'm wondering if the other condition will ever get executed?

The yarn build and yarn build:web commands both call yarn lint, so that's why that was added — and why it should prob still be there. Do you know of a good alternate way to make this work? (If the only problem is cross-env, we can prob ignore for now and fix in a separate PR.)

Security warnings for node-uuid package

We can ignore this. We could make an issue for it, but it's a warning that doesn't affect the outcome.

Increase timeout on test in check-build.test.js

I'm cool w/this.

Prepend node command to test:web

I'm also cool w/the changes you're suggesting here.

Tests failing due to default timeout being exceeded

It looks to me like the timeout here is caused by a CORS issue. For context, the way this browser build test should work is by spinning up a proxy server and then proxying the requests through that server so we don't have CORS issues. See:

      if (Merc.browser) {
        const proxyUrl = 'http://localhost:3000/';
        urls = urls.map(article => ({
          title: article.title,
          url: proxyUrl + article.url,
        }));
      }

It seems like the proxy urls aren't being used in the error you're seeing above, and that seems to be what's causing the timeout?

LOG: 'Error', 'CORS request rejected: http://www.cnn.com/2016/11/05/middleeast/iraq-mosul-isis-offensive/'

Can you look into why it doesn't seem to think it's running the browser build on this one? If it's a Windows thing, we can make this a separate issue/fix.

Two failed tests when running test:node

You're right, this is def a timezone thing. For these tests, we might need to change how the date comparison is made to account for that. (Might just want to make a separate issue/PR for this.)

@adampash adampash force-pushed the deps-upgrade branch from bb30e9b to b852d04 Jan 23, 2019

@george-haddad

This comment has been minimized.

Copy link
Collaborator

george-haddad commented Jan 23, 2019

I responded to each issue individually below, but the tl;dr here is: If it's an environment issue (Windows vs. *nix or timezone-related), let's pull that out into a separate task/PR so we can get this merged asap. Mostly I just want to make sure that the deps are working so I don't get in the way of other PRs getting merged. @pascalwatwat can you work with @george-haddad to extract the necessary pieces from this into separate task(s)?

This is already in another PR #216 and I did some messing around and I was able to mimic that command using a combo of grep and && but I experienced some weirdness. The timezone task is split out in this PR #222

The yarn build and yarn build:web commands both call yarn lint, so that's why that was added — and why it should prob still be there. Do you know of a good alternate way to make this work? (If the only problem is cross-env, we can prob ignore for now and fix in a separate PR.)

Yes, they both call lint and I assume that when they both call lint the expression is always evaluated to true unless run in a CI environment and when in a CI env it always calls lint:ci so having that expression has no value? Unless I am missing a point where $CI is set to true in a non-ci environment?

Either way those can be addressed in separate PRs as you mentioned. Just looking at this from a dependency perspective it passes sanity checks. All other issues we'll address in separate PRs.

@george-haddad
Copy link
Collaborator

george-haddad left a comment

All issues found in comments are now split as separate tasks, this should be good to go.

@adampash adampash merged commit 76d333f into master Jan 23, 2019

2 checks passed

ci/circleci: test-node Your tests passed on CircleCI!
Details
ci/circleci: test-web Your tests passed on CircleCI!
Details

@adampash adampash deleted the deps-upgrade branch Jan 23, 2019

@adampash

This comment has been minimized.

Copy link
Contributor Author

adampash commented Jan 23, 2019

Yes, they both call lint and I assume that when they both call lint the expression is always evaluated to true unless run in a CI environment and when in a CI env it always calls lint:ci so having that expression has no value? Unless I am missing a point where $CI is set to true in a non-ci environment?

To be honest, I don't exactly remember the reasoning here, but I think as long as one of the CI tasks is running the linter, then we're fine. I think I may have added this b/c I wanted to make sure I was linting before I ran a build locally, since all builds have until this point been done locally, and I wanted to know in advance if I had a lint error. Once we move builds/releases to CI, this sanity check shouldn't be necessary b/c we can build it into the build/release workflow in the CI config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment