Spec cleanup

[raphinesse]

This is part of an ongoing effort to make the tests of cordova-lib faster, more reliable and more maintainable.

This PR is mainly about maintainability and a bit about reliability. It focuses on the two suites pkgJson.spec and pkgJson-restore.spec. These are the two suites with the longest duration (taken together responsible for about 3/5 of the E2E test runtime). Furthermore, their code quality was so awful that I could reduce the LOC by over 30% respective over 50% without dropping any test cases or sacrificing any clarity (IMHO even drastically improving it).

Other highlights include:

• Use new, verbose test reporter for better information, especially in CI
• Unique, randomized directory name generated by tmpDir to prevent fs caching from affecting tests
• Proper suite-wide setting of test timeouts without affecting the global default
• Some minor improvements here and there

As I said before, this is part of an ongoing effort. In the next steps I will try to make the tests in the two pkgJson suites more focused (they are often testing more/other things than what it says in their description) and faster (will try to mock cordova-fetch which is a bit of a PITA for these E2E tests).

I'm also aware that many of the factored-out helpers in these two suites are identical. I'd like to leave them that way for now though and consider reusing them across suites after the steps outlined above.

Also, if you review the changes be sure to ignore white space changes. Diff anchoring to it/describe might also help. If someone is interested in that, I can provide more granular commits for these. But I thought it would even be worse to have a 100 commit PR 😒

[raphinesse]

Any thoughts on this? I'll assume lazy consensus if there's no feedback.

The test failure is spurious. If anyone has an idea what could have caused it, I'll be happy to investigate. The error message is very obscure.

[brodybits]

Sorry I didn't get a chance to look through this one, looks pretty important from the commit messages. I gotta say that spurious test failures do not surprise me so much.

[brodybits]

The first 6 commits (0917185..b6441d1) look fine to me. Not so easy for me to follow most of the changes after that, using -w flag doesn't seem to help me a lot. This is a bit of a tough spot since I don't have a lot of spare cycles, also would not favor continuing with lazy consensus.

I wonder if we (you) should do this cleanup in a bunch of smaller PRs that we can review with some more leisure, one at a time?

[brodybits]

I tried using --anchored=describe --anchored=it options (git 2.18.0 with help from homebrew) to look at 08459ac, gotta say it doesn't seem to help. I also tried opendiff, no better. I think I would have to do side-by-side before and after file comparison to check the changes. I think one function at a time would really help.

[raphinesse]

Thanks for taking the time @brodybits!

I can take out the first six commits so we can land it in a separate PR.

Regarding the other changes: As I mentioned, I have about a hundred commits in private branches that originally made up those changes. Sadly they are not really straightforward and producing a nice history would approximately equal redoing most of the changes from scratch. I guess you understand that I'm not really keen on doing that kind of work.

Last time I checked, I was able to produce diffs that were anchored to the different tests. I will check that again when I find the time. Also notice that I wrote extensive commit messages for these commits, trying to convey what is going on.

How do you wish to proceed with this?

[brodybits]

@raphinesse I would like to get the first 6 commits (0917185..a94a5ca) and FIXUP in a8015e6 landed asap, hereby approve these changes. (Not sure if FIXUP in a8015e6 was supposed to be squashed into another commit or not, I am fine either way.) Build started to look a little flakey once I merged #631 (the contributed change itself was green), which I would really rather not revert unless absolutely necessary, started working on followup fix in #632 and tried the wanted spec commits there.

I also hope we can get the build more consistently green at the same time.

[raphinesse]

@brodybits Anchored diff works for me (per commit, of course). Example

git show --ignore-space-change --anchored=it f9686c0bc09f

[brodybits]

What is the motivation to move the tests around in f5ffeec?

[raphinesse]

@brodybits It's about getting rid of the last of all the useless describe blocks that were originally in the spec file. I put it in a separate commit to not make the diff completely useless.

[brodybits]

Can you explain why events is removed? I guess it is not needed any longer?

[raphinesse]

@brodybits Correct, events isn't needed anymore.

Previously it was used as part of an overly complicated function to get the list of installed platforms. This is now done much simpler using listPlatforms from cordova/util.

Edit: Here's the commit: fb1bff3

[raphinesse]

@brodybits since you seem to be interested in the nitty gritty details, I prepared two branches with the detailed history for these refactorings. These are not necessarily up-to-date with the changes in this PR nor are they cleaned up (e.g. there are changes that are first introduced and then later removed again). However, they might give you a better idea of what was done.

I suggest looking at the pkgJson branch first, since I did the refactoring second and had a better idea of what was expecting me. I hope this helps.

master...raphinesse:verbose-pkgJson-spec-cleanup
master...raphinesse:verbose-restore-spec-cleanup

[brodybits]

Thanks @raphinesse, just saw your comments. I actually did my own work to break f9686c0 into smaller parts (along with f5ffeec ("merge more describe blocks"), c606e1f ("better names"), and a couple little things I would like to fix) in: master...brodybits:bb-cleanup-restore-spec-in-parts-wip1

It would be great if you can move the last commit (4d139e6 "Cleanup pkgJson.spec") into a separate PR that I (or someone else) could maybe review at some more leisure.

I can take a quick look at the branches you prepared, probably not in very deep detail. The changes do look really good in general and I think I will be taking some of the changes "in faith".

[raphinesse]

Thanks for all the review effort, @brodybits.

Frankly, I had hoped to merge the whole original PR "in faith". The preparation of this PR by itself already was a lot of work. And getting the tests to a state where I would actually be satisfied with the result will require a lot more work.

As I tried to convey in the original PR description, the two suites I refactored here were written very poorly. And although the refactor made them way more approachable and maintainable, they still are not good test suites in my book for various reasons. So IMO, they will have to undergo further radical changes.

Consequently, I did not want to spend much time waiting for this change to be reviewed in detail and polished to perfection or even formulating this very message, but rather get on with working on the problems I tried to fix here.

In the next few months, my free time will be scarce, and it's unlikely that I will spend it on something I don't really want to do.

[brodybits]

Looks like a very minor change in tested behavior from the following:

                // Remove helpers.testPlatform without --save.
                return cordovaPlatform('rm', ['browser']);

I would favor avoiding change in tested behavior during this kind of cleanup commit if possible.

[raphinesse]

I only joined the two adjacent calls to cordovaPlatform('rm', ...); into one. I wouldn't say that this changes tested behavior. Especially since restore is under test here, not cordovaPlatform.

The original code was

.then(function () {
    // Remove helpers.testPlatform without --save.
    return cordovaPlatform('rm', ['browser']);
}).then(function () {
    // Remove secondPlatformAdded without --save.
    return cordovaPlatform('rm', secondPlatformAdded);
})

[brodybits]

minor nit: I would rather see this defined as a constant at the beginning, even if it now appears only once.

[raphinesse]

I actually like this better the way it is. The function name already tells us what the argument is. So it's not really a magic number.

Something like setDefaultTimeout(4 * ONE_MINUTE_IN_MS) would be nicer, but hopefully we will be able to reduce this soon anyway.

That being said, I have no strong feelings about this. So I will change it and merge this part of the PR.

[brodybits]

I would personally be fine with merge "in faith", would be a little happier if you would first ask on mailing list with subject something like "Any objections with spec updates in faith?". I did find a couple minor things already which I just commented in the changes, would be happy to deal with them after the fact.

I was also wondering if we should consider breaking some of the spec scripts into separate files, maybe not so urgent with the cleanup.

I definitely see your point about the quality of the existing tests, definitely grateful for your efforts to clean it up.