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

Downcase the model name in the system scaffolds #50722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dwightwatson
Copy link
Contributor

Motivation / Background

I noticed there is an inconsistency in the system test scaffold where the model name is sometimes capitalised. This fixes the two instances where the model name is humanised.

test "should create user"
test "should update User"
test "should destroy User"

Considering all the other scaffolds use lowercase text I think it's right these two are downcased in the same way.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Jan 12, 2024
@zzak
Copy link
Member

zzak commented Jan 14, 2024

Does this actually change anything to what we're trying to test here? It seems like just a cosmetic change which we don't accept.

@dwightwatson
Copy link
Contributor Author

Sure it's a cosmetic change, but it's not change for the sake of change. When you generate a standard scaffold, the system test created is inconsistent within itself, and not consistent with all the other tests generated which are downcased. I think it's fair to suggest that the code generated within a scaffold should be internally consistent.

@zzak
Copy link
Member

zzak commented Jan 14, 2024

Ok, actually I thought it's better to remove downcase and then I found #43473.

I would investigate why that one test needs to be downcased and if there is a way to make these tests seem less brittle. Personally, try to avoid tests which depend on the text of a button or element because those things change and would have no impact on this test otherwise.

@dwightwatson
Copy link
Contributor Author

This isn't changing the content of the test itself. It will have no impact on the test passing.

It is changing the text description of the test itself.

@zzak
Copy link
Member

zzak commented Jan 14, 2024

Sure, but does it make sense that we shouldn't need to downcase in the first place?

@dwightwatson
Copy link
Contributor Author

I think we should be downcasing;

  • the controller tests generated by a scaffold are all
  • it seems conventional that test names are downcased

Either way - in a scaffolded system test 1 test uses a capitalized model name and 2 tests use a downcased model name. They should either all be capitalized or all downcased and the latter seems preferable.

@zzak
Copy link
Member

zzak commented Jan 15, 2024

Yeah it seems #41210 went pretty hard into downcasing the model names but still missing some spots. I'd like to get another opinion, maybe @seanpdoyle has thoughts here?

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

Successfully merging this pull request may close these issues.

None yet

2 participants