search

HuddleEng / PhantomCSS

Visual/CSS regression testing with PhantomJS
MIT License
4.72k stars 259 forks source link

Update location for comparison images when updating the screenshotRoot location #171

Closed thatblairguy closed 8 years ago

thatblairguy commented 8 years ago

Addresses Issue 169, problem with multiple tests' comparison images appearing in the first test's snapshot foldler.

jamescryer commented 8 years ago

I don't think it is correct that _results should default to screenshotRoot (if set) when update() is invoked without the comparisonResultRoot defined. I would expect options.comparisonResultRoot to fallback to the previously set _results before resolving to _src

thatblairguy commented 8 years ago

The difficulty with that behavior is that it makes the meaning of options.screenshotRoot inconsistent between calls to init() vs. update().

If screenshotRoot is set in init() (and comparisonResultRoot) is not, then the meaning is "Place the screenshots and comparison images in the same place."

On the other hand, on a call to update(), setting only screenshotRoot ends up meaning, "Place the screenshots here, but keep putting the comparison images in the old location." (A long-winded version of what I believe you said.)

The use cases I see are:

  1. All output in one location - this is essentially what's in the "coffeemachine" demo. You set screenshotRoot the one time, and all the output images end up in the same directory.

2 Screenshots for each test in separate locations - You set screenshotRoot to a new value for each test. What happens in this case is that the comparison images for the second test and later all end up stored in the location that was set for the first test. Setting screenshotRoot no longer affects this location. (That's the problem this pull request attempts to fix.)

3 Comparison images saved in specified alternate location - I haven't tried this one, but while writing this response, it occurred to me that with the current behavior, you could, at least in theory, set `comparisonResultRoot' to some temporary location.

Is scenario 3 the one you had in mind? If so, I'd suggest perhaps calling it out in the ReadMe, because otherwise that second use case seems (to me anyhow) the more likely to happen.

jamescryer commented 8 years ago

Sorry for the delay in reply.. Thanks for scenarios - I think you're right! I was thinking of the scenario where a user would put the comparison results in one place but change the src screenshot location for each test, but that scenario can be allowed by providing both values on update - it also seems to me to be a less intuitive scenario given the relationship of those values.

Worth mentioning that the fileNameGetter callback is usually used for scenarios where you need to change the directory of base images/results per test.

Cheers