Magic number in tests

Magic number 72 is used many times throughout the tests, which is actually present under ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED constant. I propose to replace that magic number either by the constant or method call.

I have never contributed to such large open source project as Rails. Please let me know your thoughts.

https://github.com/rails/rails/blob/master/activemodel/test/cases/secure_password_test.rb

https://github.com/rails/rails/blob/ff657e73f0b61a7e224a9f158467ed2ec915bd9b/activemodel/lib/active_model/secure_password.rb

I’m -1 on changing that. I think the constant 72 is used directly such that tests will fail if anybody changes MAX_PASSWORD_LENGTH_ALLOWED.

With your change in, a later change to MAX_PASSWORD_LENGTH_ALLOWED by some other contributor wouldn’t make any tests fail.

Since that constant isn’t meant to change without us thinking more about it, I’d say we keep the code as is.

Thanks for starting to contribute!

I could see an argument made for making it a constant in the test (EXPECTED_MAX_PASSWORD_LENGTH), which at least would document /why/ the number is being used.

Yep, we could do that :+1:

I would be happy to do this!

Thank you for feedback!

Not to “well actually” myself but another option might be to use MAX_PASSWORD_LENGTH_ALLOWED as the OP suggested, but have an explicit test for MAX_PASSWORD_LENGTH_ALLOWED=72… which might actually be a better approach, since it’ll isolate the exact defect case (MAX_PASSWORD_LENGTH_ALLOWED != 72) rather than have it be implied by a well-named-but-oddly-disconnected constant.

// Kerri

Yep, that sounds good too, Kerri.

Artur, go right ahead!

Should I create a ticket on Github first?

Artur, send a pull request :slight_smile: