Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

Test framework consistency #56

Merged
merged 2 commits into from Sep 13, 2018
Merged

Conversation

erisu
Copy link
Member

@erisu erisu commented Sep 12, 2018

Platforms affected

osx

What does this PR do?

Makes the testing framework consistent the testing framework used in our other platform and tooling repos.

  • Drops jasmine-node dependency
  • Adds jasmine dependency

What testing has been done on this change?

  • npm test

@erisu
Copy link
Member Author

erisu commented Sep 12, 2018

Commits cherry-picked from @brodybits' PR #49

package.json Outdated
@@ -41,7 +41,7 @@
"eslint-plugin-node": "^5.0.0",
"eslint-plugin-promise": "^3.5.0",
"eslint-plugin-standard": "^3.0.1",
"jasmine": "^2.6.0",
"jasmine": "^3.1.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"jasmine": "^3.1.0" really means "jasmine": "^3.2.0", which you can see in the changes to package-lock.json; I recall jasmine@3.2.0 not working on some other packages.

If you really meant jasmine@3.1.x then please use tilde (~) as in:

"jasmine": "~3.1.0",

If you are sure that jasmine@3.2.0 will work then please update as "jasmine": "^3.2.0" to avoid any possible confusion.

Copy link
Member Author

@erisu erisu Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is an issue with 3.2.0 but not all repos show the problem. This repo worked OK with 3.2.0 when I tested.

*Hopefully my package-lock.json was deleted.*🤣

The other repos will not be able to upgrade until the issue is fixed. Or we have to identify another way around for those tests. Being either changes in the test or code itself. Depending where the issues originate and what is required to work around their change.

Anyways, I can either

  1. Bump to ^3.2.0 as your preference.
  2. Or change to ~3.1.0 to match other repos.

There will always be confusion for those who don't understand ^. Even if set to ^3.2.0, it is possible they release a minor or patch revision and somebody decided to delete the package-lock.json to do a fresh install.

Which is preferred. Option 1 or 2?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it passes on jasmine@3.2.0 then I would prefer option 1 (bump to ^3.2.0).

I recall that package-lock.json was added to this repo in the past and is wanted in master branch of the other repos for the upcoming major release. This means that all changes to package-lock.json need to be committed whenever you install a new package or update.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will always be confusion for those who don't understand ^.

I would definitely agree, though understanding of semver should be a prerequisite for understanding and updating package*.json (at least at a basic level).

@erisu
Copy link
Member Author

erisu commented Sep 13, 2018

@brodybits Since I have updated the dep. to reflect what we talked about, "jasmine": "^3.2.0", I will go ahead and merge this in after I fix the package-lock.json conflict.

@janpio janpio added this to 🐣 New PR / Untriaged in Apache Cordova: Platform Pull Requests Sep 13, 2018
@erisu erisu merged commit cb425a9 into apache:master Sep 13, 2018
Apache Cordova: Platform Pull Requests automation moved this from 🐣 New PR / Untriaged to 🏆 Merged, waiting for Release Sep 13, 2018
@erisu erisu deleted the test-framework-consistency branch April 4, 2019 06:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Apache Cordova: Platform Pull Requests
🏆 Merged, waiting for Release
Development

Successfully merging this pull request may close these issues.

None yet

2 participants