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

Apply system properties to all test framworks #664

Merged
merged 2 commits into from
Oct 1, 2018

Conversation

Qi77Qi
Copy link
Contributor

@Qi77Qi Qi77Qi commented Oct 1, 2018

Fixes #662

Copy link
Contributor

@jvican jvican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution Qi 👍 The change looks good to me, I only have one minor suggestion.


val cls = frameworks.map(f => f.getClass.getName)

if(sysProperties.isEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we add the logger.warn from above in this part of the if expression so that the logic is clearer to follow (we can assume frameworkSpecificRawArgs.nonEmpty == true always, which makes ignoredArgs.nonEmpty == sysProperties.isEmpty. Also, let's move cls to the block of else to avoid computing it the arguments are only system properties.

Copy link
Contributor Author

@Qi77Qi Qi77Qi Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if user pass in both system properties and test framework specific args, ignoredArgs.nonEmpty and sysProperties.isEmpty won't be the same tho...if we move warn inside sysProperties.isEmpty block, some test framework specific args will just be ignored silently right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, I see what you wanted to see here. I apologize for my confusing comment. I'll submit another PR with this change plus an error message if the user passes both system properties and ignored args.

@jvican jvican added bug A defect or misbehaviour. task / test community Any change or proposal that is contributed by the Open Source Community. labels Oct 1, 2018
@jvican jvican changed the title apply system properties to all test framworks Apply system properties to all test framworks Oct 1, 2018
@jvican
Copy link
Contributor

jvican commented Oct 1, 2018

LGTM, I'll merge as soon as CI is green, thank you again!

@jvican jvican merged commit 3e91154 into scalacenter:master Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect or misbehaviour. community Any change or proposal that is contributed by the Open Source Community. task / test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants