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

(android) Add unit tests for run and retryPromise #445

Merged
merged 5 commits into from Jun 20, 2018

Conversation

Projects
None yet
4 participants
@Menardi
Contributor

Menardi commented May 31, 2018

Platforms affected

Android

What does this PR do?

I noticed the test coverage for cordova-android was rather low, so I decided to start writing some tests. This PR increases coverage from 44.69% to 47.83%. I first covered retry.js, which is a simple module that retries a promise-based function a given number of times. I also fixed a typo in this file which was mildly annoying.

The main tests I have written are in run.js, which now has 100% test coverage (up from 27%). This module handles the selection of the device or emulator to run on.

I would like to continue to add unit tests to help improve the code coverage, but am submitting this PR first to check that I am on the right track. I have used ES2015 notation for the tests to keep it more readable - I don't anticipate this to be a problem since it is supported in all recent node versions.

What testing has been done on this change?

I have run the tests using npm run unit-tests, and checked the coverage with npm run cover.

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.

@Menardi Menardi force-pushed the Menardi:android_unit_tests branch from be5b50b to e298c60 May 31, 2018

@Menardi

This comment has been minimized.

Contributor

Menardi commented May 31, 2018

It looks like the tests fail on Appveyor because I am using ES2015 features and that the Appveyor set up includes Node 4, which doesn't support these features. The tests pass on Node 6 and 8. I suggest removing Node 4 from there and adding 10. Would that be OK?

@dpogue

This comment has been minimized.

Member

dpogue commented May 31, 2018

Yes, node 4 is no longer supported so we're slowly going through all the repos and dropping it from CI while adding node 10.

I think that's what we should do here, but please do it as its own pull request to avoid this one getting cluttered with unrelated changes :)

@Menardi Menardi force-pushed the Menardi:android_unit_tests branch from e298c60 to cbb9d8c Jun 17, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jun 17, 2018

Codecov Report

Merging #445 into master will increase coverage by 3.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage   44.27%   47.63%   +3.36%     
==========================================
  Files          17       17              
  Lines        1694     1694              
  Branches      311      311              
==========================================
+ Hits          750      807      +57     
+ Misses        944      887      -57
Impacted Files Coverage Δ
bin/templates/cordova/lib/run.js 100% <100%> (+73.01%) ⬆️
bin/templates/cordova/lib/retry.js 100% <100%> (+84.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 393dad6...8e9078d. Read the comment docs.

@Menardi

This comment has been minimized.

Contributor

Menardi commented Jun 18, 2018

@dpogue Since #442 was merged (which removed support for Node 4), these tests now pass. I think this is now ready to merge 👍

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Jun 18, 2018

Great PR 👍

There is one issue though: the handling of promises in tests. Right now, failing tests would time out and you wouldn't get to see the error that caused the rejection.

To fix this, all async tests should return a promise instead of using the done callback. That way, Jasmine automatically handles resolution and rejection of these promises.

Thanks a lot for the contribution!

@Menardi

This comment has been minimized.

Contributor

Menardi commented Jun 18, 2018

Thanks for the suggestion, I didn't realise that was possible. According to the Jasmine async docs, support for returning promises was added in 2.7, but cordova-android uses 2.6. I'll look into upgrading and see if it causes any issues.

@Menardi Menardi force-pushed the Menardi:android_unit_tests branch 2 times, most recently from 7847555 to 8167646 Jun 18, 2018

@Menardi

This comment has been minimized.

Contributor

Menardi commented Jun 18, 2018

@raphinesse I updated Jasmine to the latest version (3.1.0) without any issues. I know it is suggested to squash commits in this repo, but I put the updating of this version in a second commit as I think it makes sense to keep it separate.

As for the tests themselves, I have rewritten them to return the promises instead of calling done. There are some asynchronous tests where calling done is needed (such as testing that the promise is rejected). For those tests, I have added handling for the failure cases to ensure that all the tests fail in a meaningful way (that is, avoiding timeouts where possible).

@raphinesse

raphinesse requested changes Jun 18, 2018 edited

I've done a more thorough review. I also included fixes for all my comments in an additional commit. It looks like a whole lot of changes, but most of them are simple Q -> Promise transformations. Please see my review comments for explanations to my changes.

Thanks again for your contribution and please do let me know if you're comfortable with the changes I made. I'm looking forward to your forthcoming test PRs 👍

.then(() => {
done.fail('Unexpectedly resolved');
})
.fail(() => {

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

At least @dpogue and me have a dream of someday getting rid of Q. Unfortunately, using Q instances is often necessary because old code relies on Q-specific methods like fail. Please use the methods provided by the native Promise interface whenever possible. In this case use catch instead of fail.

This comment has been minimized.

@Menardi

Menardi Jun 18, 2018

Contributor

Ah I see! I specifically used Q rather than native Promises since I saw it being used everywhere else. I will use Promises from now on.

retry.retryPromise(attempts, promiseFn)
.then(() => {
done.fail('Unexpectedly resolved');

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

You can actually use Jasmine's global fail and return retry.retryPromise(...) to get rid of done here too.

This comment has been minimized.

@Menardi

Menardi Jun 18, 2018

Contributor

I didn't realise Jasmine had a global fail, that is very convenient 👍

beforeEach(() => {
deferred = Q.defer();
promiseFn = jasmine.createSpy().and.returnValue(deferred.promise);

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

We could make this Q-less too by doing the following instead:

const promise = new Promise((resolve, reject) => {
    deferred = { resolve, reject };
});
promiseFn = jasmine.createSpy().and.returnValue(promise);
for (let i = 0; i < attempts + 1; i++) {
deferred.reject();
}

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

Looping is unnecessary and misleading here. promiseFn will return the same rejected promise every time, so you can reject your deferred once at the top of the test.

for (let i = 0; i < attempts + 1; i++) {
deferred.reject();
}
});

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

All in all, this would give the following test:

const attempts = 3;
deferred.reject();

return retry.retryPromise(attempts, promiseFn).then(
    () => fail('Unexpectedly resolved'),
    () => expect(promiseFn).toHaveBeenCalledTimes(attempts)
);
run.help();
expect(message.indexOf('Usage:') > -1);

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

You are not actually testing anything here. Should be:

expect(message).toMatch(/^Usage:/);

This comment has been minimized.

@Menardi

Menardi Jun 18, 2018

Contributor

Whoops, that's embarrassing!

it('should print out usage and help', () => {
let message = '';
spyOn(console, 'log').and.callFake(msg => { message += msg; });
spyOn(process, 'exit');

This comment has been minimized.

@raphinesse

raphinesse Jun 18, 2018

Contributor

This clobbers the global console.log and process.exit for the whole process. I'm not too comfortable with that. Could cause very obscure errors for other tests.

This comment has been minimized.

@Menardi

Menardi Jun 18, 2018

Contributor

I was worried about that when writing this, and looked into how to unspy a function in Jasmine, but it seems that Jasmine automatically unspies after a test is run. So, console.log and process.exit would not be spied in other functions. Personally I would prefer to explicitly unspy to avoid this confusion, but from what I can find, there is no way to do that.

This comment has been minimized.

@raphinesse

raphinesse Jun 19, 2018

Contributor

Interesting, I did not know that spies get unspied. OTOH it's a good thing, but OTOH I agree that explicit operations are preferable at times. Conveys intent and informs unsuspecting users like me 😅

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Jun 18, 2018

Another note: when we finally have unbundled dependencies, we probably should replace retryPromise with p-retry to reduce our maintenance burden.

So next time when you come across an untested homemade util, before investing time to write tests for it, consider looking for a lib that does the same job and include it instead. Having to maintain less code is always preferable. 😇

@Menardi Menardi force-pushed the Menardi:android_unit_tests branch from 781bbba to 98de909 Jun 18, 2018

@Menardi

This comment has been minimized.

Contributor

Menardi commented Jun 18, 2018

Thanks for the detailed review, I really appreciate it. I would have looked into replacing retryPromise but I didn't want to be ripping things out in one of my first commits. I won't hesitate next time though :)

I have replied to most of your comments. On the topic of spying on console.log, I want to reiterate that Jasmine's behaviour is not what you might expect. I spent a while when originally writing the tests trying to figure out how to unspy, so that other tests would not be affected by the spy. But actually Jasmine unspies automatically after the test. I confirmed this by checking if console.log was spied in a following test, and it was not.

I am happy with your changes, and again very much appreciate your input. I will work on writing some more tests in another branch during the week.

By the way, I rebased the branch with master as there was a conflict in package.json.

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Jun 19, 2018

I would have looked into replacing retryPromise but I didn't want to be ripping things out in one of my first commits.

Understandably so 😅

Given the temporary nature of global spies, please feel free to revert back to using them if you want. My bad for not knowing better.

For me, this is good to go. I'd probably squash all commits, but I don't really care either way here. @dpogue what do you say?

Thanks again for this great PR and any following ones 👍

@dpogue

dpogue approved these changes Jun 20, 2018

👍

@raphinesse raphinesse force-pushed the Menardi:android_unit_tests branch 2 times, most recently from d2f8d64 to 69e5481 Jun 20, 2018

Add unit tests for run and retryPromise
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>

@raphinesse raphinesse force-pushed the Menardi:android_unit_tests branch from 69e5481 to 8e9078d Jun 20, 2018

@raphinesse raphinesse merged commit 559b083 into apache:master Jun 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Menardi Menardi deleted the Menardi:android_unit_tests branch Jun 21, 2018

brodybits added a commit to brodybits/cordova-android that referenced this pull request Jun 21, 2018

brodybits added a commit to brodybits/cordova-android that referenced this pull request Jun 21, 2018

brodybits added a commit to brodybits/cordova-android that referenced this pull request Jul 4, 2018

brodybits added a commit to brodybits/cordova-android that referenced this pull request Jul 10, 2018

brodybits added a commit that referenced this pull request Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment