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

GPII-2927: Fixed code coverage for process reporter components. #604

Merged
merged 6 commits into from May 4, 2018

Conversation

Projects
None yet
4 participants
@klown
Copy link
Contributor

commented Apr 4, 2018

@the-t-in-rtf I have modified the HTML test fixtures as you suggested. Code coverage is much improved. Do you have time to review? Thanks!

klown added some commits Apr 4, 2018

GPII-2927: Fixed code coverage for process reporter components.
HTML test fixtures now include the coverage client to transmit coverage results
back to the coverage servers.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2018

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

@klown, I don't have permission to assign things or to perform a merge, but I'm happy to leave comments.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

@klown, I wanted to raise some points about the code coverage results, but can't link to line numbers because nyc isn't new enough. I'd suggest running npm outdated while you're there and at a minimum updating nyc, testem, and gpii-testem to the latest released versions. I did this locally, and saved a copy of the report here:

https://the-t-in-rtf.github.io/coverage-reports/universal/20180405/index.html

I wanted to ask you about a couple of branches that aren't reported as tested. In particular, I wanted to ask you about this one:

https://the-t-in-rtf.github.io/coverage-reports/universal/20180405/universal/gpii/node_modules/processReporter/src/processesBridge.js.html#L186

For whatever reason a process isn't returned, IMO you should be able to confirm that the system handles that appropriately. It may be that another fixture needs to be set up to report coverage, or that there simply aren't tests. If there aren't tests, let's talk through how hard it would be to add them, hopefully it's the kind of thing you can take care of in passing?

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Also, things like these shouldn't be included in the report:

https://the-t-in-rtf.github.io/coverage-reports/universal/20180405/universal/gpii/node_modules/transformer/test/js/index.html
https://the-t-in-rtf.github.io/coverage-reports/universal/20180405/universal/gpii/node_modules/settingsHandlers/test/web/js/index.html

I'll try generating "node only" and "web only" reports to see who's creating those, depending on the results, we'll either need to update the NYC config or the gpii-testem component options. More on that in a bit.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

OK, the node tests are fine, it's clearly the web tests that bring in the test content. I'll suggest a fix shortly.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

If you add the following to the nonSources option in testem.js, the extraneous test content will be excluded:

            "./**/*Tests.js",
            "./**/*TestEnv.js",
            "./**/*TestsUtils.js",
@klown

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@the-t-in-rtf Thanks for the comments! I think I have addressed them with the changes I've made -- have a look.

Specifics:

I wanted to ask you about a couple of branches that aren't reported as tested. In particular, I wanted to ask you about this one:
...
hopefully it's the kind of thing you can take care of in passing?

No, it's simple to add a test for that, and the other lines that were marked in red. I'm not sure about the lines marked with "Else path not taken", since there is no "else" path to take.

If you add the following to the nonSources option in testem.js, the extraneous test content will be excluded:

          "./**/*Tests.js",
           "./**/*TestEnv.js",
           "./**/*TestsUtils.js",

Sure, but isn't that a broader, global issue than improving coverage for the process reporter components? Perhaps another JIRA?

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2018

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Sure, but isn't that a broader, global issue than improving coverage for the process reporter components? Perhaps another JIRA?

Hi, @klown. I understand what you mean, but I often am asked to (and take care of) small stuff that is unrelated to my immediate work as part of an active PR, and I tend to think it's fair game to fix small broken windows when we notice them. IMO it's more likely to result in immediate action than adding to our backlog of longstanding unreviewed pulls. For things that are outside the scope of the immediate work, I frequently create a ticket, and commit to the branch with that ticket's issue key, linking that ticket to the existing pull.

I'm not sure about the lines marked with "Else path not taken", since there is no "else" path to take.

Whether or not you have a literal else block, whenever you have an if which is always hit, the system correctly reports that you have not ever confirmed that the system works without traversing that if block. The most serious case is one where you return from an if, where obviously the implied else behaviour is different simply by virtue of execution continuing beyond the if block at all.

To keep this specific, let's look at this example from the process bridge coverage report:

https://the-t-in-rtf.github.io/coverage-reports/universal/20180405/universal/gpii/node_modules/processReporter/src/processesBridge.js.html#L158

That to me indicates that although you allow passing procArray as an optional argument, you don't ever test that use case. This is important enough to at least ask:

  1. If people might usefully avoid the if branch, why isn't that scenario tested?
  2. If the if branch is unlikely to ever be avoided in real-world usage, why does it exist?

In some cases, we address this kind of gap by agreeing that the implicit else needs tests. In other cases, we agree that the if itself is not needed, and collapse that so that the contained code is always executed. In still other cases, we leave things as they are and live with the reduced branch coverage. This last outcome happens a lot with failure checks.

In the case I shared above, I think it's important to talk about why we allow that optional argument, and if we agree it's useful, to add tests to cover the implicit else.

GPII-2927, GPII-2936: Improving code coverage.
- Better code coverage for process reporter components,
- Added additional irrelevant files to testem's "nonSources" option.
@klown

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

Hi @the-t-in-rtf,

I understand what you mean, but I often am asked to (and take care of) small stuff that is unrelated to my immediate work as part of an active PR, and I tend to think it's fair game to fix small broken windows when we notice them.

Fair enough: I've created GPII-2936, and added the to-be-excluded-files to testem.js as you specified above.

Whether or not you have a literal else block, whenever you have an if which is always hit, the system correctly reports that you have not ever confirmed that the system works without traversing that if block.

D'oh! My question was literally and simply (and naively), "there is no else-block to check so how can I write a test for it?". But, as you noted, there is an implicit else in these cases. Of course there is. Chalk it up to it being late on a Friday afternoon... And, that's why you have reviewers, folks. Anyhow, I have added coverage for all the implicit else-blocks listed in the report.

In the case I shared above, I think it's important to talk about why we allow that optional argument, and if we agree it's useful, to add tests to cover the implicit else.

Here's the rationale: for an actual OS*, it's relatively expensive to get the list of processes -- the procArray -- so, if you just generated it, you can use it again. On the other hand, the list becomes stale relatively quickly, so if you think the list is out-of-date, don't pass it in. It is left to the client to determine the trade-off between expense and accuracy.

*- in these (universal) tests, the list is a mock and in memory, and getProcessList() is blazing fast.

New version coming up...

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2018

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

Ugh, the last passing CI build was actually a failure with a bunch of NullPointerException messages. Confirming locally and checking out the updated coverage report.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

Thanks, @klown. With your additional tests and screening out the test content, the report is much clearer:

https://the-t-in-rtf.github.io/coverage-reports/universal/20180410/index.html

IMO the remaining gaps are fine to talk about in future pulls working in those areas, and nothing that should block this pull from being reviewed further.

I also confirmed the tests passing locally. Ok to test again in CI.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Thanks @the-t-in-rtf

Ugh, the last passing CI build was actually a failure with a bunch of NullPointerException messages.

Weird. As you said, there are NullPointerExceptions in the general console log, but the logs for the browser and node tests show that all tests were run and passed (32 and 1256, respectively). Indeed the other console logs show that everything is tickety-boo. CI is schizoid.

I'm going to look more closely at the process reporter tests in the windows and linux branches for similar coverage issues that the work here revealed. I'll be using the tools in your GPII-2493 pull.

Thanks again.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Hi, @klown. Sorry about the timing here, looks like the underlying branch was just merged with master. Probably it's best to close this and submit a (linked) PR against the GPII repo.

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2018

Hi @the-t-in-rtf . You mean that your GPII-2483 branch/pull for code coverage in Windows was merged. The pull request here (GPII-2927) is for Universal and is based its master branch. Still, it's a bit out of date, so I've merged in the latest from universal master, and pushed.

(And, I'll go rebase my Windows pull request as you suggest).

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2018

GPII-2927: Improve code/test coverage.
Added wildcards to include the "userListeners" module -- the
tests were run, but the results were not included in the coverage
report since the code was not listed in .nycrc.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2018

@amb26 amb26 merged commit 35f1695 into GPII:master May 4, 2018

1 check passed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.