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

Start 8.1.x minor release #693

Merged
merged 69 commits into from Sep 14, 2018
Merged

Start 8.1.x minor release #693

merged 69 commits into from Sep 14, 2018

Conversation

@brodybits
Copy link
Contributor

@brodybits brodybits commented Sep 14, 2018

Platforms affected

All

What does this PR do?

Start 8.1.0 minor release, based on changes from master through 22 August, with the following additions:

  • minor workaround fixes needed to work on deprecated Node.js 4 version
  • update to use cordova-windows@~6.0.x by default (GH-691)

Motivations for minor release:

  • resolve npm audit issues that show up in 8.0.0, as already done in master branch
  • Include some AppVeyor CI/Travis CI stability & performance improvements from master, including 30% shorter runtime in fetch.spec.js (3be60ef)
  • use cordova-android@~7.1.x by default (GH-646)
  • use cordova-windows@~6.0.x by default (GH-691)
  • Small desire to include the few minor changes to src along with the other changes in master up to 3be60ef (30% shorter runtime in fetch.spec.js)

What testing has been done on this change?

  • npm audit shows 0 vulnerabilities
  • coho audit-license-headers -r lib shows no missing license headers
  • coho check-license -r lib shows no invalid licenses
  • Travis CI is green on @brodybits fork
  • Travis CI is green in this PR
  • AppVeyor CI is green in this PR

How to merge

In case this proposal is accepted, it is desired to push these commits into a new 8.1.x branch. @brodbits would like to handle this step.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change. (Updated to pass on deprecated Node.js 4)
stevengill and others added 30 commits Dec 14, 2017
This should be replaced at some point with a more dynamic method of looking up platform engines.
Dependency cleanup
Also removed before_install from Travis configuration since Node 6
already comes with npm 3 there
CB-14065: Drop support for Node 4 and add Node 10 to CI
…ers and update cordova-browser to 5.0.3
There was a lot of sub-optimal error handling in the tests.

Instead of handling Promise rejection and resolution ourselves, we now
just return them from the test functions and let Jasmine handle them.

Moreover, this also fixes tons of negative tests that looked something like this:

    f().then(function () {
        expect('foo').toBe('bar');
    }).catch(function (err) {
        expect('' + err).toMatch('some message');
    });

Problems being:
- `expect('foo').toBe('bar')` is less obvious than `fail(...)`
- catches its own Error from the `.then` only to fail because `err` is missing
- Does not check that err is an Error and that the message is in `.message`

So these were normalized to look like this:

    f().then(function () {
        fail('Expected promise to be rejected');
    }).catch(function (err) {
        expect(err).toEqual(jasmine.any(Error));
        expect(err.message).toMatch('some message');
    });

All in all, this commit has the following benefits:
- Removes 700 LOC w/out removing any testing
- Improves output when async tests fail
- Should make async tests less error prone
- Reduces Q-API usage to prepare for removal of Q (CB-14159)
These tests tended to cause spurious fails on CI.
Hopefully, this commit will stabilize them.

Noteworthy changes include:
- Reset test files before each test
- Don't disguise setup/teardown as tests
- Test 020 was dropped since it was actual a weaker version of 019
- Compare objects instead of JSON strings
- Don't use reserved words as variable names
- Reduce test duration by about 50%
- Fix linting errors
- Remove shelljs usage
- DRY and simplify code
- 200 LOC less
One test in pkgJson-restore.spec did NOT wait for the promise returned
by `prepare` to be resolved before continuing with other tasks.
Especially on Windows this could lead to EBUSY failures during cleanup
and in turn to failures during following tests.

Note: The order of emptyPlatformList() and prepare() does not seem to matter.
Thus I chose the variant that was used throughout all other tests.
CB-13055 Fold all fetch options to `true`
This uses junctions whenever creating directory symlinks on Windows
since that does not require any special privileges.
* Improve plugman/uninstall.js messages
* Adapt plugman_uninstall.spec.js to new error messages
Chris Brody
CB-14243: change dash to underscore for save-exact key reference
This new reporter gives a better idea which tests are currently running,
how long individual tests take and during which tests output occurred.

All of this information is especially helpful when tests are run in CI.
raphinesse and others added 21 commits Aug 15, 2018
See #644
Previously, the timeout defined by setDefaultTimeout did not apply for
beforeAll and afterAll blocks. This change fixes that.

The new implementation makes use of the fact that Jasmine processes
beforeAll in FIFO order and afterAll in LIFO order.
Minor cleanup of the imports in HooksRunner.spec. No functional changes.
Test 012 sporadically takes slightly longer than the current default
timeout of 10s, so this increases the timeout for this test to 20s.

Related to #642
Covers a few more cases and reduces maintenance for us.
Only warn about "too long shebang" if the file really starts with one
This partially reverts commit 61395d2
but keeps Node.js 10 in AppVeyor CI and Travis CI
- fs-extra@4 needed to support deprecated Node.js version 4
- fs-extra currently not used outside of spec & integration-tests
@brodybits brodybits self-assigned this Sep 14, 2018
Copy link
Contributor

@raphinesse raphinesse left a comment

I don't really see the need for this with us working on the next major release, but the changes look good 👍

@brodybits brodybits changed the base branch from start-8.1.x-proposal to 8.1.x Sep 14, 2018
@brodybits brodybits merged commit e75d18c into apache:8.1.x Sep 14, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brodybits brodybits deleted the brodybits:start-8.1.x-proposal branch Sep 14, 2018
@brodybits brodybits changed the title Proposal: start 8.1.x minor release Start 8.1.x minor release Sep 14, 2018
brodybits pushed a commit to brodybits/cordova-lib that referenced this pull request Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants