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

Console returns 0 for invalid fixtures #1379

Closed
ChrisMaddock opened this Issue Mar 27, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@ChrisMaddock
Copy link
Member

ChrisMaddock commented Mar 27, 2016

Separated out from #1346, if the console encounters invalid test suites, it still returns zero. Invalid assemblies and invalid tests contribute to the return code, but not invalid test suites.

Should we add an additional return code for this? -3 FIXTURE_NOT_FOUND currently appears to be unused - what was the intention of that, for invalid filtering?

public static readonly int OK = 0;
public static readonly int INVALID_ARG = -1;
public static readonly int INVALID_ASSEMBLY = -2;
public static readonly int FIXTURE_NOT_FOUND = -3;
public static readonly int UNEXPECTED_ERROR = -100;
@CharliePoole

This comment has been minimized.

Copy link
Member

CharliePoole commented Mar 27, 2016

As mentioned on the other issue, I thought we had fixed this. Looking at the code in ConsoleRunner, it appears not.

FIXTURE_NOT_FOUND was an error we gave when the obsolete /fixture option was used and no such fixture was found. We no longer give an error when no tests are found but perhaps we should, changing the name of course.

In principle, if a fixture is bad, we should return the number of tests that would have been run. However, depending on the nature of the error, we may not know what that number is. Simply counting fixture failures as well as test cases would work for some purposes but it sounds a bit sloppy. So I would say we need an extra negative code for this.

A crazy alternative I just thought of: count every bad fixture as 1000 errors. A result like 1002 would mean 1 bad fixture and 2 failing test cases. It breaks down for over 999 failures, but so what? Drawback - which is probably why it's crazy - is that every CI runner might need to interpret it.

@CharliePoole CharliePoole added the is:bug label Mar 27, 2016

@CharliePoole CharliePoole added this to the Backlog milestone Mar 27, 2016

@CharliePoole

This comment has been minimized.

Copy link
Member

CharliePoole commented Mar 27, 2016

@ChrisMaddock Educational note! :-) I marked it as a bug, because it seems like one to me... good enough reason. I did not mark it as requiring confirmation (status:confirm) because I was able to verify that's what the code does by a quick glance. I assigned it to Backlog, 'cause that's what we do.

Hope this helps.

@ChrisMaddock

This comment has been minimized.

Copy link
Member

ChrisMaddock commented Mar 28, 2016

@CharliePoole Is there a specific advantage to using a positive return code here? Im not so keen on the ambiguity...1001 meaning either 1001 tests failed, or 1 test and one suite failed. Is it also correct that suites can contain other suites, so we cant even be sure of the number of failed suites? I realise so long as its non-zero, the actual code is less important, but we have a nice simple return code structure at the moment, and this feels like its complicating things.

It feels to me like we should return a negative code if any part of 'discovery' failed, like this is here. Thoughts?

@ChrisMaddock ChrisMaddock self-assigned this Mar 28, 2016

@CharliePoole

This comment has been minimized.

Copy link
Member

CharliePoole commented Mar 28, 2016

IIRC we have code to only count errors that originated in the fixture. However, I agree with you about adding a new negative code.

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