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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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