Page MenuHomePhabricator

Add an acceptance test for Celerity maps
ClosedPublic

Authored by joshuaspence on Jul 4 2014, 5:19 AM.
Tags
None
Referenced Files
F14003073: D9817.id23558.diff
Sat, Oct 26, 1:53 AM
F13993764: D9817.diff
Wed, Oct 23, 1:16 AM
F13976989: D9817.id.diff
Fri, Oct 18, 4:36 PM
F13972972: D9817.id23665.diff
Thu, Oct 17, 10:09 PM
F13968794: D9817.id.diff
Wed, Oct 16, 11:01 PM
F13957808: D9817.diff
Mon, Oct 14, 11:25 AM
Unknown Object (File)
Sep 22 2024, 10:49 PM
Unknown Object (File)
Sep 20 2024, 3:41 PM
Subscribers

Details

Summary

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.

Test Plan

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

Repository
rP Phabricator
Branch
arcpatch-D9817
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1592
Build 1593: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Add an acceptance test for Celerity maps.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
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.

epriestley edited edge metadata.

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.

This revision is now accepted and ready to land.Jul 4 2014, 2:45 PM
joshuaspence edited edge metadata.

Refactored CelerityResourceMapGenerator

joshuaspence edited edge metadata.

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.

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"

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.

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.

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.

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?

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.

Yeah, I'm not sure what I meant anymore... I'll revert to the original implementation with a separate MapGenerator class.

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?

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.

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.

Has there been any discussion as to how .arcunit would work? I might be able to throw something together.

Let me file something and we can come up with a plan there.

How about I put it in src/__tests__/PhabricatorCelerityTestCase.php for now and we deal with T5568: Support `.arcunit`, similar to `.arclint` separately?

joshuaspence edited edge metadata.
  • Restore CelerityResourceMapGenerator
  • Move Celerity acceptance test to PhabricatorCelerityTestCase
epriestley edited edge metadata.

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).

This revision is now accepted and ready to land.Jul 9 2014, 9:38 PM
This revision was automatically updated to reflect the committed changes.