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

Adds negative test of Enum.HasFlag #131

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

SandorDobos
Copy link
Contributor

Description

Supports nanoframework/Home#730

Motivation and Context

How Has This Been Tested?

Executed the test method :)

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (work on Unit Tests, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Could you please reuse the existing enum flags and adapt the test methods.
I've recently removed the repeated enum flags that where there.
There is no point on adding new enums just because... If the one there is suitable for the test lets use it. DRY and KISS, please. 😉

@SandorDobos
Copy link
Contributor Author

I need two enums because this test requires two separate enum types.
I could reuse the existing enum instead of one of them but that will make the test less readable because the second enum still has to be defined somewhere. Should I do this even so?

@josesimoes
Copy link
Member

I need two enums because this test requires two separate enum types.
I could reuse the existing enum instead of one of them but that will make the test less readable because the second enum still has to be defined somewhere. Should I do this even so?

I've missed your point on using 2 enums. Now I can see that.
Then please "duplicate" the existing enum and do exactly what you're doing. This was that second enum will be available for news tests than need some something similar.
Does this make sense to you?

@SandorDobos
Copy link
Contributor Author

Done

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks.

@josesimoes josesimoes changed the title Adds negative test of Enum.HasFlag. Adds negative test of Enum.HasFlag Apr 13, 2021
@josesimoes josesimoes merged commit 290ca6c into nanoframework:develop Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enum.HasFlag throws ArgumentException
3 participants