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

Refactor test cases of AnnotateRoutes - completed version #760

Merged

Conversation

nard-tech
Copy link
Collaborator

This is the final completed version of refactoring AnnotateRoutes.

@nard-tech nard-tech requested a review from drwl February 21, 2020 14:47
@nard-tech
Copy link
Collaborator Author

@drwl @ctran Thank you for your cooperations.

Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

Looks good - just had 2 questions. The first one about expect not being hit was just a comment. The second question on dfdffc5 is the one I'm interested in.

@@ -307,6 +307,8 @@
end

it 'should skip annotations if file does already contain annotation' do
expect(File).not_to receive(:open).with(ROUTE_FILE, 'wb').and_yield(mock_file)
expect(mock_file).not_to receive(:puts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering - would it ever get to this expectation? I thought it would fail on L310 before it ever hits L311.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drwl
Oh, I am not cofident in whether L311 makes sence.
But L310 is an expectation for confirming that the File does not receive openmethod, so it should not cause any failure, I think.

context 'with older Rake versions' do
let :rake_routes_result do
<<~EOS.chomp
(in /bad/line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's supposed to be going on here?

Copy link
Collaborator Author

@nard-tech nard-tech Feb 24, 2020

Choose a reason for hiding this comment

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

@drwl
The variable defined by let :rake_routes_result is just cut-and-pasted.

This variable has been existed before I begin to refactor the test cases.
It seems to be since 69cd69c (spec/annotate/annotate_routes_spec.rb).
I can't figure out what this commit means because this commit is not recorded as GitHub PR.

Maybe this test does not make sense for result of rake routes by recent Rails version, so I'd like to rewrite test cases around this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, I'll merge it in for now and feel free to remove in future work.

@nard-tech nard-tech requested a review from drwl February 24, 2020 20:01
Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

Your work is appreciate 👍

@drwl drwl merged commit dce2ac6 into ctran:develop Feb 25, 2020
@nard-tech nard-tech deleted the feature/refactor_annotate_routes/rspec_v4 branch February 25, 2020 15:18
vfonic pushed a commit to vfonic/annotate_models that referenced this pull request May 8, 2020
This is the final completed version of refactoring AnnotateRoutes.
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