Revert "remove unused test files"#7915
Conversation
This reverts commit 8862cd2.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Note: I already updated my PR to not need those files. Don't have a strong opinion on what we should include in test data. Generally I think our manually curated set was intentionally curated. Agree we should discuss it. |
|
I included I included The other ML model data files were included in order to exercise more code paths. I'm not sure whether they were actually being used to exercise those code paths. Not sure about ethiopic-amete-alem. |
robertbastian
left a comment
There was a problem hiding this comment.
My cleanup removed 13MB of unused test text files from the repo. It didn't remove any tests, cause any tests to fail, or resulted in a diff in testdata. There is absolutely no reason to revert it.
I included
hawa long time ago because it exercises the romanlow date format, and I think, at least at the time, it isn't in the bakeddata, so the test needs this data in order to run in datagen.
The test does not exist. If you need a specific file in an upcoming PR, just add it back.
I included
csfor a similar reason. I feel fairly certain that we had a test for this data at some point, and it's possible that the test got dropped during one of the refactorings of legacy datetime.
I didn't remove "cldr-dates-full/main/cs/ca-gregorian.json" because that test uses it. It just doesn't use "cldr-dates-full/main/cs/timeZoneNames.json".
The other ML model data files were included in order to exercise more code paths. I'm not sure whether they were actually being used to exercise those code paths.
These have been unused since we only generate testdata for Thai models in #3669.
Not sure about ethiopic-amete-alem.
Sure about what? It's obviously unused, otherwise there would have been a data diff. Is it a bug that it's unused, and it should be checked to match ethiopian? Maybe, in which case you're welcome for discovering that, and we should add it back when we fix this.
|
I'm okay with the cleanup overall. Occasionally checking why we have things and deleting if not is good, and it seems like we have a few things here we shouldn't have deleted but "let's remove unused things and readd a few important ones" is an okay way to do it. When reviewing I didn't notice |
|
There is one straightforward reason to revert this, though. When reviewing I said I wanted Shane to look at it, you had an urgency on the PR, and I said fine. My repeatedly-held position is that is is good to land things fast, but the corollary of that is we need to be okay with disagreements in followups. This is a pretty cleanly separable followup. That said, this is internal code that doesn't affect API; so it really doesn't matter much if we land this PR first, discuss it, and then land something that does it partially, or if we discuss it on this PR, tweak it, and land some subset. The latter option is fewer PRs. |
|
We discussed this and agreed to add back files when they are needed. Also, we opened #7925 to track the ethiopic-amete-alem data. |
This reverts commit 8862cd2.
This is a clean revert of the commit in #7906 that deleted the files @Manishearth was using.
Changelog
N/A