Conversation
erisu
force-pushed
the
increase-code-coverage
branch
from
911b5b0 to
934f9ea
Compare
brody2consult
changed the title
tests/spec/unit/versions.spec.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('should find ios-sim version.', (done) => { |
There was a problem hiding this comment.
For some reason versions.get_tool_version('ios-sim'), versions.get_tool_version('ios-deploy'), and versions.get_tool_version('pod') do not work for me when I test on my local system, don't know why. Any clues?
I would also favor moving these into a separate script.
There was a problem hiding this comment.
Are these tools installed locally? These tests can only run on a Darwin environment and with these tools installed.
I did not mock the child process exec so it an actual test, but does not care what is the exact version.
Both locally and on the Travis CI environment has these tools are installed. I did not configure Travis CI to install them. Either the environment already came with the tools or was installed via npm install. From package.json or the .travis.yml script step.
I could mock the data, but by mocking, I would not have discovered that the Mac OSX SDK changed to macOS SDK.
There was a problem hiding this comment.
As for separate scripts, do you mean something like a bash file? In /cordova-ios/bin there are some individual script files that calls onto versions.js to fetch the value for an individual item.
For example:
/cordova-ios/bin/apple_ios_version
/cordova-ios/bin/apple_osx_version
/cordova-ios/bin/apple_xcode_version
But it is missing the ios-sim, ios-deploy, and pod specific executable script.
There was a problem hiding this comment.
No I meant separate .spec.js file.
I will try npm i --save-dev ios-deploy and see if that solves at least part of the problem.
There was a problem hiding this comment.
npm i --save-dev ios-deploy seems to do the trick, except for versions.get_tool_version('pod'); I just pushed the change to your branch
There was a problem hiding this comment.
Adding ios-deploy to package.json as a devDep will cause AppVeyor to fail. This package can only be installed and used on the Darwin platforms.
I question if testing iOS/macOS platform on AppVeyor is actually needed. Xcode is a build requirement and Windows cant use Xcode, and AppVeyor is running on Windows, what is achieved with this test.
npm ERR! code EBADPLATFORM
npm ERR! notsup Unsupported platform for ios-deploy@1.9.3: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"ia32"})
npm ERR! notsup Valid OS: darwin
npm ERR! notsup Valid Arch: any
npm ERR! notsup Actual OS: win32
npm ERR! notsup Actual Arch: ia32
There was a problem hiding this comment.
Also, should this dep be added to the platform?
Per documentation, it's expected that the user installed it manually and globally.
- Having it bundled with the platform might mean fewer setup steps.
- Could the globally installed
ios-deployversion be affected by which platform versions is installed on the project? If so, maybe it should be dep.
https://cordova.apache.org/docs/en/latest/guide/platforms/ios/#installing-the-requirements
| "xml-escape" | ||
| ] | ||
| ], | ||
| "nyc": { |
There was a problem hiding this comment.
If I would do npm install it writes the following change to package.json:
index 7b21884a..ee6b352a 100644
--- a/package.json
+++ b/package.json
@@ -69,7 +69,12 @@
"xml-escape"
],
"nyc": {
- "include": ["bin/templates/scripts/**"],
- "reporter": ["lcov", "text"]
+ "include": [
+ "bin/templates/scripts/**"
+ ],
+ "reporter": [
+ "lcov",
+ "text"
+ ]
}
}I would favor having the "nyc" entry committed this way.
There was a problem hiding this comment.
Are you referring to the formatting, each item on its own line? I can make this change.
There was a problem hiding this comment.
Yes. I have a few other general remarks coming on this nice PR.
(was part of apacheGH-377) covers update from apacheGH-376 Co-authored-by: エリス <ellis.bryan@gmail.com> Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
|
Really nice in general. My major comment is that I am personally not ready to break deprecated Node.js 4 compatibility for a couple reasons:
I think we can keep deprecated Node.js 4 compat a couple straightforward changes:
I think a valid alternative for using |
(was part of apacheGH-377) covers update from apacheGH-376 Co-authored-by: エリス <ellis.bryan@gmail.com> Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
tests/spec/unit/versions.spec.js
Outdated
| expect(version).not.toBe(undefined); | ||
| done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This case assumes that pod is installed. Easy solution for me is to install using homebrew as discussed in CocoaPods/CocoaPods#2238 (comment).
I have mixed feelings about this test. Positive is that it verifies that versions.get_tool_version() works on globally installed pod tool. Negative is that it may break some developers ability to try npm run unit-tests before raising PRs.
(was part of apacheGH-377) covers update from apacheGH-376 Co-authored-by: エリス <ellis.bryan@gmail.com> Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
|
@erisu I took the liberty to include a small part of this PR in GH-384 (#384) that I just merged in order to add coverage for the changes merged from #376. I recommend that you rebase on master whenever you can. I think you can see that I took a few more minor liberties on the title, description, and added commits to solve a few things. As a side point I was able to get most of the changes working on |
|
With the recent mailing list discussion, to mark the next major Cordova dev release (cordova-ios@5.0.0-dev), Some repos have already dropped Node 4 support and began using let, const, class etc in master. For example cordova-android. I would prefer a uniform coding syntax usage across all repos and not having a one-off restriction on what can be used. I know that the drop support task will take time and create temporary restrictions/differences until completed. As for the transitioning of var to let and const, I had been making these changes as I go along, with the understanding that Node 4 is soon to be dropped. I had not planned to do a mass search and replace though. I also agree and excited to see the committed node_modules removed. I have always been wondering behind this.
The only concern I had with the other PR was the I haven't looked too much into why it was decided. |
erisu
force-pushed
the
increase-code-coverage
branch
from
9baf297 to
815ea13
Compare
erisu
force-pushed
the
increase-code-coverage
branch
from
815ea13 to
0a8e258
Compare
Codecov Report
@@ Coverage Diff @@
## master #377 +/- ##
==========================================
+ Coverage 65.62% 73.68% +8.05%
==========================================
Files 14 11 -3
Lines 1702 1463 -239
Branches 286 0 -286
==========================================
- Hits 1117 1078 -39
+ Misses 585 385 -200
Continue to review full report at Codecov.
|
|
@erisu I think this is good, hope we can deal with a few minor points:
I would be fine if you want to finish and merge this one yourself, if you want me to review again, or even if you would like me to take this one over. |
|
@brodybits When you have some time, can you review this one more time? Especially the last three commit. The last three commit should cover the items from your previous message.
Regards with the second item point, I also agree that users may not install external dependencies such as pods and ios-deploy but should be able to run unit testing. In this case, I introduced component testing. This is a good example of component level testing where it involves two or more components. I didn't go with e2e for this because its not a full end-to-end test.
Regards to the last item, commit cleanup, can you explain what is intended? Typically I squash my commits but since it was mentioned to not squash, I am trying to figure out how to clean up in what way. Please let me know your thoughts. |
|
@erisu now I understand a few things. My one major criticism is that I think I personally do not really like squashing multiple kinds of changes together, like both migrating to nyc and adding more test coverage. More things at the same time could be more to grasp at the same time. Major factor is that if someone wants to back-port to an earlier release, separate commits would be nicer. But this is just my personal opinion, some others seem more likely to squash.
|
Replaced outdated istanbul with Istanbul nyc (Fixes nodejs 10 coverage run issue)
- Completed remaining versions.js testing for full coverage - Added Jasmine Coverage Configuration - Added component test execution to npm test
erisu
force-pushed
the
increase-code-coverage
branch
from
6d5086d to
7d96cb5
Compare
|
I would like to add another comment that I think the concept of unit test vs e2e integration test is currently not consistent between all Cordova repos, and is actually not so clear to me. For example: it is actually not so clear to me what defines "spec" test vs e2e integration test in cordova-lib. Noting this here before it gets forgotten. |
|
@brodybits Yes, I also see sometimes inconsistencies across all repos. Basically, I see testing broken into three categories. This is how I view testing in general. What tasks or open action items we should do from these comments is something I am not sure about, as of yet. Maybe we just need document the difference and when to pick one over the other. We should update the repos at least to be consistent with the structure. Sorry if it sounds a bit generic and maybe textbook. Unit Testing, which are the test specs and I feel in most cases these are done well. Component Testing is testing between two or more components. For example our previous situation with the pods and ios-deploy tools. Component testing may also involve environment configurations to ensure that the test work. One good old example is the Application + Database E2E Testing is testing the entire system as a whole. How we define E2E, I am also not clear. It seems in this repo, E2E represents if the basic app builds. In most cases, I feel E2E starts from the highest point in how end users typically use Cordova. In this case, I would think all of our E2E tests would be within CLI. There are other testing categories like UI testing. |
|
I also want to update package.json. But I did not know when, where, and should it be brought up. Some of it might be a bit opinionated though. 1. Not all commands are consistent between each repo. Example: 2. (opinionated) The script naming convention. would become This is opinionated so there is no priority and if no action, it is all good. 3. (opinionated) Order of the testing. becomes Should linting actually be a pretest instead of post? The order of the tests seems reversed IMO. E2E tests take the longest time to run. It should be the last. If a unit test fails, it is likely that the e2e tests will fail. The CI I believe stops at the failure? Correct me if I am wrong. If the e2e ran first, fails, and stop the CI, we had to wait for the test to finish first, and then we have to investigate where in the process it failed. Prefered Order: unit -> component -> e2e |
|
Those testing categories seem to make sense, would need some more time to better understand. Should ideally be discussed somewhere else if it affects multiple repos. Changes approved on my part. |
(was part of apacheGH-377) covers update from apacheGH-376 Co-authored-by: エリス <ellis.bryan@gmail.com> Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
3 participants
Platforms affected
iOS
What does this PR do?
I wanted to help increase the code coverage for iOS so I wrote some additional tests around the missing area.
I also:
istanbul@^0.4.2with their new maintained Istanbul nyc packagenyc@^12.0.2.This PR requires PR #375 and #376 to be merged before hand. I will rebase this PR as the other PRs are merged.
What testing has been done on this change?
Note: The links provided below contains results with the required PRs, mentioned above, merged.
Checklist
Reported an issue in the JIRA databaseCommit 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.