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

Moved Grammar unit tests #67

Merged
merged 4 commits into from
Aug 28, 2023
Merged

Conversation

lucasmenezesds
Copy link
Contributor

Main Changes

  • Commit

    • Moved Grammar unit tests for it's own folder and changed the name to represent what was really testing (symbol_spec)
    • Added the setting for randomizing the orders the unit tests run
    • Added the setting to allow focusing tests (but there is an attention point)
  • Commit

    • Removed the constant definition to the let statement instead
    • Removed extra describe (RSpec.describe Lrama::Grammar do)
    • Changed to use described_class instead of Lrama::Grammar::Symbol
  • Commit

    • Created a simple unit test for Code#translated_code

Kernel.srand config.seed

# Allow to limit the run of the specs
config.filter_run_when_matching :focus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attention point for having this enabled is that if someone forgets the focus on the code, then this would be a problem since most of the tests would be skipped.

So I'd like your opinion on this matter

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to NOT set this configuration because it's risky and costly to check each PR does not introduce focus method (e.g. fit). Could you remove this configuration?

end
end
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add new line on the end of a file.


# Randomize specs run order.
config.order = :random
Kernel.srand config.seed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this code bit tricky. I guess the intention is to fix seed of Kernel#rand. However IIRC at the moment there is no dependency for such randomness.

lucasmenezesds and others added 4 commits August 27, 2023 18:28
- Moved and renamed file from `spec/lrama/grammar_spec.rb` to `spec/lrama/grammar/symbol_spec.rb`

- Added randomization to rspec tests and option to allow focusing test cases
- Removed extra describe
-  Changed to use described_class instead
@lucasmenezesds lucasmenezesds requested a review from yui-knk August 27, 2023 22:00
@yui-knk yui-knk merged commit e82f4da into ruby:master Aug 28, 2023
11 checks passed
@yui-knk
Copy link
Collaborator

yui-knk commented Aug 28, 2023

Thank you!!

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.

2 participants