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

auto-generate tests for attributes in work resource generator #4348

Merged
merged 1 commit into from
May 29, 2020

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented May 29, 2020

Confirms that the generated work resource responds to all generated metadata.

Related to PR #4347

@samvera/hyrax-code-reviewers

Confirms that the generated work resource responds to all generated metadata.
return unless attributes.present?
inject_into_file filepath, after: /it_behaves_like 'a Hyrax::Work'\n/ do
"\n context 'includes schema defined metadata' do\n"\
"#{attributes.collect { |arg| " it { is_expected.to respond_to(:#{arg.name}) }\n" }.join}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this needs to be wrapped in string interpolation

Suggested change
"#{attributes.collect { |arg| " it { is_expected.to respond_to(:#{arg.name}) }\n" }.join}" \
attributes.collect { |arg| " it { is_expected.to respond_to(:#{arg.name}) }\n" }.join

has the same result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried creating it without and it didn't work. Since I'd made several tweaks getting this to work, I tried it again, and it still doesn't work. Perhaps with much fiddling around I could figure out either why it doesn't work or how to make it work. Not sure it is worth it. I did a quick test in irb following the same pattern of multi-line string with block in the middle and it also failed with a syntax error.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it is possible to put this all inside the template and avoid the injection?

Copy link
Contributor

Choose a reason for hiding this comment

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

the injection is conditional on attributes.present?, but maybe it should not be?

i.e. what if we put the outline, including the context block in the template, plus a # it { is_expected.to respond_to(:my_attribute}) line as guidance for app developers. then let the injection take place only for the custom attrs.

Copy link
Contributor

Choose a reason for hiding this comment

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

in either case. i'm approving this. i think we should merge as-is and refine from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjcolvar How would you put that in the template? The attributes are defined inline with the generator. You could put an example in the template, but I’m not sure that really buys us anything. I don’t know another way to add to a template other than using inject_into_file.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but I'd assume that the arguments would be available for use within the template so you could generate it just like you are now but in ERB style.

@no-reply
Copy link
Contributor

looks good to me. i made a simplifying suggestion.

@cjcolvar
Copy link
Member

I'm excited by a generator that can actually build a fully working work type!

@cjcolvar cjcolvar merged commit da8055f into master May 29, 2020
@cjcolvar cjcolvar deleted the gen_schema_tests branch May 29, 2020 19:42
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.

3 participants