search

FredrikNoren / ungit

The easiest way to use git. On any platform. Anywhere.
MIT License
10.44k stars 637 forks source link

Report coverage using npm scripts #1365

Closed Hirse closed 4 years ago

Hirse commented 4 years ago

Follow up from #1363.

@campersau I'm not sure, but it seems like the tests don't always terminate when running with coverage. I thought that would be fixed with #1357.

Should we add coverage to the build scripts as well?

part of #895

campersau commented 4 years ago

For me this works fine when all tests pass.

But for example the credentials-helper tests sometimes don't pass when I am having a crashed ungit instance opened in chrome because I stopped the server and the crashed ungit pages tries to reconnect by sending polling requests with socket.io which then the node server in the credentials-helper test picks up and throws inside of the request handler which means that the server does not a response and eventually times out. So to prevent this edge case we could add a timeout to the node response so it times out faster. https://github.com/FredrikNoren/ungit/blob/d58bc4a0fea8d8b9d5aee29b836a56b5c2cf020f/test/spec.credentials-helper.js#L13-L23

Do you see other cases when this hangs?

We could add --exit to the mocha tests but then we might hide potential issue such as this and #1357

With build scripts do you mean in CI?

Hirse commented 4 years ago

I've seen it not terminate if tests fail.

I have my git set up to sign my commits, so during the tests it would also prompt for my password which usually leads to a timeout.

Yes, I mean whether to report coverage during CI builds.

campersau commented 4 years ago

Okay, so the timeouts are expected then. The git-promise uses a two minute timeout by default. https://github.com/FredrikNoren/ungit/blob/fa0e117a59e415005d13bb7cca02aa9ce430ac0b/source/git-promise.js#L158 Have you tried if --exit fixed it for you?

Not sure how discoverable it currently would be to output the coverage in CI. Maybe when also adding it as a comment to the PR it makes more sense? But we can do that in another PR if we want to do it.

campersau commented 4 years ago

There are some CI cancellations now because of tests hangs:

So we might need to add --exit for now because I haven't reproduced these hangs yet. But that's not related to this PR.