Allow usage of the generate action in generators when testing#34418
Allow usage of the generate action in generators when testing#34418tubbo wants to merge 1 commit intorails:masterfrom
generate action in generators when testing#34418Conversation
There was a problem hiding this comment.
Have you tried rails instead bin/rails? It always shellout and it was working and for the report you have it was failing because bin/rails doesn't exist in the execution directory but rails will work in any directory.
There was a problem hiding this comment.
I didn't investigate that much but changing the run_ruby_script("bin/rails ...") to run_ruby_script("rails ...") produces the same problem (i.e. No such file or directory -- rails).
On the other hand, relying on system("rails generate ...") properly triggers the generation but the file are placed inside the application's app and test directories rather than the specified source_destination in the test.
There was a problem hiding this comment.
@rafaelfranca yes, @robin850 is correct...I tried your suggestion of changing bin/rails to rails, but I got the same issue in tests. Additionally, when actually trying to run it in the real world, I got an additional "No such file or directory" error printed to STDOUT. That said, I don't believe the ability to test the results of a generator called from another generator ever worked, at least I can't remember a time when it did...
There was a problem hiding this comment.
On the other hand, using Generators.invoke rather than shelling out removes support for code doing something like:
generate("scaffold articles --no-timestamps")While it looks like the test suite isn't relying on such case and the documentation doesn't mention it, people might rely on this feature. But I don't know whether we should have something like:
unless args.present?
what, args = a.split(" ", 2)
endOr deprecate this ability.
There was a problem hiding this comment.
If we want to test this we should not be stubbing anymore, this test give no guarantees this will work.
There was a problem hiding this comment.
@rafaelfranca agreed, I don't believe this assertion is every useful...would you be OK with me replacing the test to be this?
action :generate, "model", "MyModel"
assert_file('app/models/my_model.rb')That way we're actually testing the results of the operation rather than whether some command was called...
|
also...I targeted this for |
|
oh, right, we don't merge things on stable branches unless the bug doesn't exist on master. It doesn't seems to be the case. |
It was previously not possible to test whether a generator could invoke another generator using the `generate` command, because this shells out and can potentially be mocked or its errors can be hidden from the user. The command always runs, but its effects cannot be observed. For example, if a file is supposed to be generated using `generate :model`, the file will not exist in the `destination_root` of the test when executed. To remedy this, instead of shelling out with `#run_ruby_script`, use `Generator.invoke` directly and pass in all of the arguments given to the command. Fixes rails#33558
d6f2aba to
6a8f5d2
Compare
|
@rafaelfranca ahh ok, thanks for clarifying! I re-based it to 'master' |
|
@deivid-rodriguez It would seem so! And your approach seems less "breaking" than mine, since I'm changing the entire way @rafaelfranca you mind if I close this in favor of #34420? it seems to solve a couple more problems than just the |
|
@tubbo Great! Yeah, my PR should basically keep things as they are now, but you can opt-in to a more strict behavior via the new |
|
No at all. I'll review #34420. |
|
I fail to see how aborting the generator solves the original problem, which still exists to this day as far as I can tell. |
Summary
It was previously not possible to test whether a generator could
invoke another generator using the
generatecommand, because thisshells out and can potentially be mocked or its errors can be hidden
from the user. The command always runs, but its effects cannot be
observed. For example, if a file is supposed to be generated using
generate :model, the file will not exist in thedestination_rootofthe test when executed. To remedy this, instead of shelling out with
#run_ruby_script, useGenerator.invokedirectly and pass in all ofthe arguments given to the command.
Other Information
(none)