Use configurable path printer in JSON and JUnit formatters #1677
Conversation
|
|
||
| $outputPrinter->extendFeatureAttributes([ | ||
| 'name' => $this->currentFeature->getTitle(), | ||
| 'name' => $this->currentFeature->getTitle() ?? '', |
There was a problem hiding this comment.
I noticed that this can be null and if that is the case we need to print an empty string
There was a problem hiding this comment.
Or I suppose we could make the name element nullable in the JSON schema?
But I think you're probably right that it's simpler for end-users if we coalesce to an empty string - I doubt there's a usecase for being able to explicitly tell whether a feature name was provided..
| private readonly ResultToStringConverter $resultConverter, | ||
| private readonly JUnitOutlineStoreListener $outlineStoreListener, | ||
| private readonly ?JUnitDurationListener $durationListener = null, | ||
| private readonly ?ConfigurablePathPrinter $configurablePathPrinter = null, |
There was a problem hiding this comment.
This needs to be made nullable to keep BC
|
@acoulton as promised here is the implementation of what we discussed yesterday |
acoulton
left a comment
There was a problem hiding this comment.
Great work, thanks @carlos-granados
|
|
||
| $outputPrinter->extendFeatureAttributes([ | ||
| 'name' => $this->currentFeature->getTitle(), | ||
| 'name' => $this->currentFeature->getTitle() ?? '', |
There was a problem hiding this comment.
Or I suppose we could make the name element nullable in the JSON schema?
But I think you're probably right that it's simpler for end-users if we coalesce to an empty string - I doubt there's a usecase for being able to explicitly tell whether a feature name was provided..
| $testCaseAttributes['file'] = | ||
| str_starts_with($file, $cwd) ? | ||
| ltrim(substr($file, strlen($cwd)), DIRECTORY_SEPARATOR) : $file; | ||
| if ($file && $this->configurablePathPrinter instanceof ConfigurablePathPrinter) { |
There was a problem hiding this comment.
So if someone has somehow created a JUnitScenarioPrinter outside our DI container (without the path printer) we will just stop including a file attribute in the junit.
I think that's unlikely to happen, and the file attribute was already optional so the report is still schema-valid, so I think this is OK.
There was a problem hiding this comment.
Yeah, that was my thinking
|
We'll need to note in the changelog that the junit |
18d20ac to
1a76cb2
Compare
The JSON and JUnit formatters were using a very simple management of the printed file paths. This PR replaces this with the ConfigurablePathPrinter that the other formatter use. This brings a couple of advantages:
%paths.base%to calculate relative paths, this is more correct than using cwd as those formatters were doingThe editorUrl option of that printer is not appropriate for these formatters, which cannot use the link added to the paths, so I added a flag to mark if this option needs to be applied or not
Adds tests that check the correct working of the printer options in these formatters