Extend Language ID tests to prevent re-use and manual generation #4315
Conversation
And prevent the reuse of deleted IDs
| @@ -385,7 +385,8 @@ def test_all_languages_have_a_language_id_set | |||
| end | |||
|
|
|||
| def test_all_languages_have_a_valid_id | |||
| invalid = Language.all.select { |language| language.language_id < 0 || language.language_id >= (2**31 - 1) } | |||
| deleted_language_ids = [21] # Prevent re-use of deleted language IDs | |||
There was a problem hiding this comment.
21 was removed in #3933. This prevents re-use "to be safe".
| @@ -385,7 +385,8 @@ def test_all_languages_have_a_language_id_set | |||
| end | |||
|
|
|||
| def test_all_languages_have_a_valid_id | |||
| invalid = Language.all.select { |language| language.language_id < 0 || language.language_id >= (2**31 - 1) } | |||
| deleted_language_ids = [21] # Prevent re-use of deleted language IDs | |||
| invalid = Language.all.select { |language| language.language_id < 0 || deleted_language_ids.include?(language.language_id) || (language.language_id > 431 && language.language_id < 1024) || language.language_id >= (2**31 - 1) } | |||
There was a problem hiding this comment.
431 is the last manually generated ID. 1024 is picked at random to catch attempts to manually set an ID. I also contemplated using (2**20 - 1) as a lower limit but that's also just picked at random with no qualification.
I was going to implement this as:
language.language_id != Digest::SHA256.hexdigest(language.name).to_i(16) % (2**30 - 1)... however this won't work when a language name changes as the ID determined in this test would no longer be based off the original SHA generated off the original name and adding a list of "exemptions" feels a little 🤢 to me.
I'm open to suggestions on how best to catch most attempts at setting an ID manually.
There was a problem hiding this comment.
Is there no way to run tests on diffs? It would allow us to check for a lot more mistakes, including the correctness of IDs.
There was a problem hiding this comment.
Is there no way to run tests on diffs?
There is, but that could get quite hairy and time consuming and would mean shelling out or getting fancy with something like Rugged.
If the gen'd ID has a predictable format or guaranteed minimum size (in value or number of chars), that could be an alternate and more robust solution too.
There was a problem hiding this comment.
We could write a Ruby script to validate the git diff HEAD^, but I'm not sure it's worth it here. I'll investigate later, when I'll have more time.
There was a problem hiding this comment.
We could write a Ruby script to validate the
git diff HEAD^, but I'm not sure it's worth it here. I'll investigate later, when I'll have more time.
👍 I looked into this but it was quite slow, especially if we want to catch staged and unstaged files for testing locally before committing.
I'll leave that in your capable hands - when you have time - and merge this for now. We can iterate if we need to.
Description
As discovered in #4314 our current tests don't detect if someone has manually incremented the language ID instead of using
script/update-ids. This PR attempts to implement changes to catch this scenario.Checklist:
Template doesn't really apply, but gonna keep this as the closest.