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

36 Unit tests #461

Merged
merged 4 commits into from
Aug 1, 2024
Merged

36 Unit tests #461

merged 4 commits into from
Aug 1, 2024

Conversation

Anatolay
Copy link
Collaborator

@Anatolay Anatolay commented Jul 30, 2024

Closes #36.

This PR adds:

  • An option to consider only the first output of normalization step during unit tests
  • Several unit tests for rules in yegor.yaml

PR-Codex overview

This PR introduces a new RuleTestOption type, allowing for test-specific options in the EO Phi rules. It also modifies test cases to include these options for more flexibility.

Detailed summary

  • Added RuleTestOption type for test-specific options
  • Updated test cases to include RuleTestOption for more flexibility

The following files were skipped due to too many changes: eo-phi-normalizer/src/Language/EO/Phi/Syntax/Doc.txt

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@Anatolay Anatolay linked an issue Jul 30, 2024 that may be closed by this pull request
1 task
@0crat
Copy link

0crat commented Jul 30, 2024

@Anatolay It is not a good idea to name Git branches the way you named this one: "unit-tests". You've earned -10 points. Next time, better give your branch the same name as the number of the ticket that you are solving. In this case, a perfect name, for example, would be "460". Your running balance is +5.



%% File generated by the BNF Converter (bnfc 2.9.5).
The Language Syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file changed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you change file encoding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change applied automatically when I pulled and built master

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Diif comparison shows that it only added blank lines, no other changes



%% File generated by the BNF Converter (bnfc 2.9.5).
The Language Syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you change file encoding?

@fizruk fizruk requested a review from deemp July 30, 2024 12:00
}
deriving (Generic, FromJSON, Show)

newtype RuleTestOption = TakeOne {take_one :: Bool}
Copy link
Member

@deemp deemp Jul 30, 2024

Choose a reason for hiding this comment

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

You should rename TakeOne to RuleTestOption.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assumed there could be more options in the future. I can rename for now, though

@deemp
Copy link
Member

deemp commented Jul 30, 2024

@Anatolay, #36 required 5 unit tests for each rule. Please add more unit tests

@Anatolay
Copy link
Collaborator Author

@Anatolay, #36 required 5 unit tests for each rule. Please add more unit tests

Wasn't it discussed that 2-3 per rule should be enough?

@deemp
Copy link
Member

deemp commented Jul 30, 2024

@Anatolay, it was discussed, but not agreed.

@deemp
Copy link
Member

deemp commented Jul 30, 2024

@Anatolay, it was agreed to write at least 3 tests.

@Anatolay Anatolay changed the title Unit tests 36 Unit tests Jul 30, 2024
@fizruk
Copy link
Collaborator

fizruk commented Aug 1, 2024

Let's merge this, and follow up with more unit tests if necessary, in a separate PR.

@fizruk fizruk merged commit 6f6cdd0 into master Aug 1, 2024
8 checks passed
@fizruk fizruk deleted the unit-tests branch August 1, 2024 16:08
@0crat
Copy link

0crat commented Aug 1, 2024

@fizruk Thanks for the review! You've earned +49 points for this: +25 as a basis; +34 for the 340 hits-of-code that you reviewed; -10 for too few (3) comments. Your running balance is +189.

@0crat
Copy link

0crat commented Aug 1, 2024

@Anatolay Thanks for the contribution! You've earned +23 points for this: +30 as a basis; -7 for too many hits-of-code (340 >= 100). Please, keep them coming. Your running balance is -5.

@0crat
Copy link

0crat commented Aug 10, 2024

@Anatolay Thanks for your review! While we appreciate your effort, reviewing your own contribution isn't recommended. You've earned +5 points: +25 base, -40 for self-review, +20 minimum adjustment. Remember, external reviews often provide valuable insights. Your current balance is +23. Keep up the good work, and consider inviting others to review your future contributions!

@0crat
Copy link

0crat commented Aug 10, 2024

@deemp Hey there! 👋 Great job on the review! You've snagged +42 points for this one: +25 for being an awesome reviewer, +6 for tackling those 340 hits-of-code (that's a lot!), and +11 for your 11 insightful comments. 💬 Your effort really shows, especially since you avoided the -10 point deduction for having less than 6 comments. Keep up the fantastic work! Your running balance is now +130. Remember, the more you contribute, the more you earn. Looking forward to your next review! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests
4 participants