-
Notifications
You must be signed in to change notification settings - Fork 52
RAT-465: fix core tests after removal of deprecated objects #576
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
base: FEAT_Remove_deprecations
Are you sure you want to change the base?
RAT-465: fix core tests after removal of deprecated objects #576
Conversation
e1e1aad to
051894e
Compare
e429be2 to
bf5a190
Compare
| */ | ||
| public static ReportConfiguration parseCommands(final File workingDirectory, final String[] args, | ||
| final Consumer<Options> helpCmd, final boolean noArgs) throws IOException { | ||
| final Consumer<Options> helpCmd, final boolean noArgs) throws IOException, ParseException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseException seems to be missing in Javadoc.
| configuration.getClaimValidator().listIssues(reporter.getClaimsStatistic())))); | ||
| } | ||
| } | ||
| } catch (ParseException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we log something before exiting with status 1?
| generateConfig(ImmutablePair.of(option, args)); | ||
| assertThat(DefaultLog.getInstance().getLevel()).isEqualTo(level); | ||
| } catch (IOException e) { | ||
| } catch (IOException | ParseException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we let ParseException extend IOException in order to simplify the code here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't own ParseExcepton. It is from commons cli.
| System.setOut(origin); | ||
| } | ||
| } | ||
| // @OptionCollectionTest.TestFunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a TODO here in order to make it easier to find the spot that needs more work ;) Thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is removed as it was a duplicate.
| fail(e.getMessage(), e); | ||
| } | ||
| } | ||
| // @OptionCollectionTest.TestFunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a TODO here in order to make it easier to find the spot that needs more work ;) Thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed as it was duplicated code.
|
@Claudenw thanks for all your work - little review comments attached. Let's see how things evolve while fixing the other modules! At least it simplifies many things and makes RAT cleaner. Thx |
|
@ottlinger If you are happy with the changes please merge and I will get the maven one queued up for review. |
Fixes for core tests to adapt to removed options, methods and classes.
This fixes the issues with the core build and test on the FEAT_Remove_deprecations branch.
this PR will not build. It is intended only to fix the core module.
The build should successfully build core and fail on tools.