Fixes T5374. Add an acceptance test to the PhabricatorInfrastructureTestCase class which fails if a Celerity map is not up-to-date. In order to achieve this, a lot of code used to generate Celerity maps was transferred from CelerityManagementMapWorkflow to CelerityResourceMap and CelerityResourceMapGenerator.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5374: Write an acceptance test for detecting when the Celerity map file is out-of-date
- Commits
- Restricted Diffusion Commit
rP212b0d29c54f: Add an acceptance test for Celerity maps
Ran arc unit and noticed that all tests passed. Modified a JavaScript file and ran arc unit again (without running ./bin/celerity map)... this time the test failed, as expected.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/__tests__/PhabricatorInfrastructureTestCase.php | ||
|---|---|---|
| 35 ↗ | (On Diff #23558) | These assertions could be made a lot smarter and raise better error messages. |
| src/infrastructure/celerity/CelerityResourceMapGenerator.php | ||
| 3–14 | I don't really like having a separate class for this. Probably better to move all of this code into CelerityResourceMap. | |
Comment Actions
That lint issue is probably just an out-of-date libphutil. Only one guy who hit a similar possible-break lint issue so I think he might just have had mismatched versions, but I'll dig into it and file something or shoot you a patch if I see another hit.
I vaguely wonder if a better approach here might be adding a dedicated arc artifacts command, which we would run during arc diff, after arc patch, and during arc land. But I would guess most projects don't have anything like this, and ideally they shouldn't (at some point, it would maybe be nice to switch Phabricator to an internal, artifact-free repository -- but using git for release management has a lot of advantages, and the costs imposed on us by having all this map stuff are annoying but not very large.
Comment Actions
Does this still fly with you? It makes the CelerityResourceMap class much more complicated unfortunately, but it minimises a lot of duplication between CelerityResourceMap and CelerityResourceMapGenerator.
Comment Actions
I haven't touched this code in a while, but is it plausible to put this stuff in some new CelerityResourceMapGenerator class instead of directly in the Map class? If that's a pain, I think this is fine as-is.
| src/infrastructure/celerity/CelerityResourceMap.php | ||
|---|---|---|
| 260 | stray "0" | |
Comment Actions
That's what I had done originally (see original diff), but it didn't feel right to me. I guess I wanted to make this more like the PhutilLibraryMapBuilder. I'm fine either way.
Comment Actions
Doesn't have a separate generator class make it more like MapBuilder, which is itself a separate generator class (vs PhutilBootloader, which is more like the map class -- sort of)?
I'm totally fine with either approach. In the abstract, I have a very mild preference for separating generation, but not for any particularly strong reason.
Comment Actions
One issue is that the test won't run if assets (webroot/rsrc/**/*) are modified, due to the directory structure. Perhaps moving this test to webroot/rsrc/__tests__/ would be appropriate?
Comment Actions
Yeah, I'm not sure what I meant anymore... I'll revert to the original implementation with a separate MapGenerator class.
Comment Actions
Which, of course, wouldn't get picked up in the library map. I don't have any great ideas on how to solve this without an .arcunit file format which could be used to define more complicated source-test mappings.
Comment Actions
We could also theoretically move webroot/rsrc/ to src/rsrc/ -- there's no reason that stuff needs to be in webroot/ anymore.
I think .arcunit is probably a cleaner approach in the long run, though.
Comment Actions
Has there been any discussion as to how .arcunit would work? I might be able to throw something together.
Comment Actions
How about I put it in src/__tests__/PhabricatorCelerityTestCase.php for now and we deal with T5568: Support `.arcunit`, similar to `.arclint` separately?
Comment Actions
- Restore CelerityResourceMapGenerator
- Move Celerity acceptance test to PhabricatorCelerityTestCase
Comment Actions
Yeah, that's fine with me.
| src/infrastructure/celerity/CelerityResourceMapGenerator.php | ||
|---|---|---|
| 91 | This was wrong before (PhutilNumber should be used with %s); it's now right in some senses, but will lose the nice "1,234" formatting. Slightly more correct/nice would be %s + new PhutilNumber(...), which lets us produce "1,234" for large numbers (or locale-aware equivalent, eventually). | |