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

Use up-to-date fixtures in tests #770

Merged
merged 18 commits into from Oct 10, 2019

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Apr 13, 2019

Motivation and Context

  • Ensures we test cordova-lib against our latest platform releases/project structures
  • No more outdated versions of our platforms committed to source control
    • Tracked files down from 1725 to 387
    • ~170k LOC less in source control

Resolves #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.

@raphinesse raphinesse added this to the 9.0.2 milestone Apr 13, 2019
@raphinesse raphinesse requested review from dpogue and erisu April 13, 2019 14:17
@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Apr 13, 2019
@raphinesse
Copy link
Contributor Author

Hmmm, seems that Travis won't run for draft PRs 🤔

@raphinesse
Copy link
Contributor Author

raphinesse commented Apr 13, 2019

If anyone with a Windows Machine could have a look at that Node 6 error that occasionally pops up, that would be great.

@raphinesse raphinesse marked this pull request as ready for review October 8, 2019 21:46
@erisu
Copy link
Member

erisu commented Oct 9, 2019

@raphinesse the node issue you are seeing is a known issue with the version of fs-extra we have set for this repo.

apache/cordova#121

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.

Copy link
Member

@erisu erisu left a 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
Copy link
Member

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

Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Oct 9, 2019
@erisu
Copy link
Member

erisu commented Oct 9, 2019

Also the CI tests were failing for missing shelljs module. I think after that one test is fixed this is good to be merged.

@raphinesse
Copy link
Contributor Author

@erisu thanks for the review. I'll try to find out what's going on with the shelljs dependency errors.

The changes from this commit should be able to go in any upcoming release.

@raphinesse raphinesse closed this Oct 9, 2019
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to ☠️ Closed/Abandoned Oct 9, 2019
@raphinesse raphinesse reopened this Oct 9, 2019
Apache Cordova: Tooling Pull Requests automation moved this from ☠️ Closed/Abandoned to 🐣 New PR / Untriaged Oct 9, 2019
@raphinesse
Copy link
Contributor Author

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.

@raphinesse
Copy link
Contributor Author

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.

@raphinesse
Copy link
Contributor Author

raphinesse commented Oct 9, 2019

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.

@erisu Yes please! Let's target next major. This will make a few things easier moving forward.

@raphinesse
Copy link
Contributor Author

OK, nice. So now we are left with only one fs-extra related error for Node 6 on Windows. That's good enough for me to merge.

@raphinesse raphinesse merged commit 293b9c0 into apache:master Oct 10, 2019
Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to 🏆 Merged, waiting for Release Oct 10, 2019
@raphinesse raphinesse deleted the up-to-date-fixtures branch October 10, 2019 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Development

Successfully merging this pull request may close these issues.

Use up-to-date fixtures in tests
2 participants