Paths

Table of Contentst

Differential D9817

Add an acceptance test for Celerity maps
ClosedPublic
Actions

Authored by joshuaspence on Jul 4 2014, 5:19 AM.

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

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 updated this revision to Diff 23558.Jul 4 2014, 5:19 AM

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.

joshuaspence added a task: T5374: Write an acceptance test for detecting when the Celerity map file is out-of-date.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 4 2014, 5:19 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B1514: Diff 23558.Jul 4 2014, 5:20 AM

joshuaspence added inline comments.Jul 4 2014, 2:35 PM

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

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

Refactored CelerityResourceMapGenerator

Harbormaster completed remote builds in B1563: Diff 23631.Jul 7 2014, 5:34 AM

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"

In D9817#16, @epriestley wrote:

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?

In D9817#18, @epriestley wrote:

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.

In D9817#19, @joshuaspence wrote:

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?

Restore CelerityResourceMapGenerator
Move Celerity acceptance test to PhabricatorCelerityTestCase

Harbormaster completed remote builds in B1592: Diff 23665.Jul 9 2014, 9:31 PM

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