-
Notifications
You must be signed in to change notification settings - Fork 243
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
Use up-to-date fixtures in tests #770
Conversation
Hmmm, seems that Travis won't run for draft PRs 🤔 |
1d440a6
to
b9303b0
Compare
If anyone with a Windows Machine could have a look at that Node 6 error that occasionally pops up, that would be great. |
7824507
to
ec5cace
Compare
@raphinesse the node issue you are seeing is a known issue with the version of In the draft branch that I been preparing for the next major bumps this dependency to the current released version that was promising to resolve this issue. Were you planning to release this PR for the next major or in a minor? If next major, I can start creating PRs from the commits from my draft branch and merge them in. This would resolve the issue but as well we stop testing Node 6 and 8 in the next major. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Overall LGTM
Just that one file missing header.
@@ -0,0 +1,9 @@ | |||
#!/usr/bin/env node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to Header the Apache Software Foundation License
Also the CI tests were failing for missing |
@erisu thanks for the review. I'll try to find out what's going on with the The changes from this commit should be able to go in any upcoming release. |
1ac1ca0
to
14aeddf
Compare
Had to be kept in devDeps before because of some outdated fixtures
14aeddf
to
28bdc0c
Compare
OK. So I'm starting to pull out my hair over this one. That one error is very mysterious. It happens consistently in Travis runs of this PR. I cannot reproduce it locally, neither could @erisu. When I pushed the exact same commit to another branch in my fork, the error vanished and I could not even once reproduce it there. Right now, I'm at a loss here. |
I don't know what caused the error but I've been able to avoid it. by using a different fixture for the test in question. There's still the Node 6 failure though. |
@erisu Yes please! Let's target next major. This will make a few things easier moving forward. |
OK, nice. So now we are left with only one |
Motivation and Context
cordova-lib
against our latest platform releases/project structuresResolves #757
Description
This introduces a central helper to get test fixtures from. The fixtures cannot be directly accessed by tests but must instead be copied to a target directory specified by the test. The fixtures (mostly projects of different nature) are created using our own tools and up-to-date platforms upon first requesting them. Later requests for the same fixture re-use the formerly created instance to reduce test duration.
All outdated and now unused "static" fixtures have been deleted.
cordova-test-platform
is required as a dev dependency but does not seem to be released at all. We could either do that, or at least tag a new version in the repository and reference that. I'm open to suggestions.