feat(baseline): add --baseline-output option for the disk baseline provider#3616
feat(baseline): add --baseline-output option for the disk baseline provider#3616mpownby wants to merge 2 commits into
Conversation
|
@mpownby I don't think a new option is warranted here, instead stryker should follow the outputpath parameter for disk baseline provider, it is then auto-ignored because it will be covered by a gitignore. |
ReproductionThe disk baseline provider writes its report to 1. Clone the repo and restore the Stryker toolgit clone https://github.com/RulecityLLC/DotNetMutationTestingSandbox.git
cd DotNetMutationTestingSandbox
dotnet tool restoreReproduces on any recent 4.x and on current 2. Move into the test projectThe documented baseline workflow runs from the test project directory, so this is the realistic case: cd RestApi.Unit.Test3. Seed a baseline, explicitly redirecting Stryker's output elsewheredotnet stryker --with-baseline:89b7eb28a64d1fa3f418faf99760e6d2a4bf63e5 --version main --output ../redirected-stryker-output
Inspect the result — two things to notice:
4. Run again incrementally — the baseline should be reused, but isn'tdotnet stryker --with-baseline:main --version main --output ../redirected-stryker-outputThe log shows The problem and proposed solutionThe disk baseline provider currently ignores
|
c042cdc to
e277cec
Compare
My first pass at this PR was to have disk baseline provider following --output if the user explicitly set it (or ignore it otherwise) but I was worried about breaking any existing users. If you'd rather me go back to this solution, I can do that. |
|
Baseline and custom output directory are currently functionally at odds as you justifiably reported. Since we've never had this report before, it's unlikely it is actually being used together much in practice. I would class this as an oversight/bug when custom output directory was introduced. The two options are simply incompatible right now, and this must be addressed. It would also be acceptable to me if disk baseline provider places it's own .gitignore in the baseline directory instead of following the output directory, however I expect that users expect that output directory affects all files generated by stryker anyway. Beside that we could also additionally support a new config parameter to explicitly set a baseline output path, that is also a valid ask. |
If disk baseline follows the custom output directory, then another flag needs to be added to indicate whether the user set a custom output directory, because by the time the disk baseline runs, the output directory will be set to either an auto-generated one (with timestamp) or a user-supplied one, and it will fail if it writes to an auto-generated timestamp path. I think if few are relying on this feature, then the 'right' solution is to make it follow the output path if the user explicitly sets it. If you agree, I can rework this PR to go down that path. |
|
I'm considering whether or not we should move full output path generation including timestamp back to stryker.core and should only pass the working directory or custom output directory from stryker.cli, then output directory can be used directly in baseline. I am not yet sure if that is worth it. |
e277cec to
691047a
Compare
|
I'm also not in favor of adding a new option for this. Especially when setting the path in the settings file it could cause unexpected behavior when the relative path can not be resolved on another system. It just seems like a bug to me that this file is not outputted in the output directory. I suggest we place the file in the root of the StrykerOutput dir, not in a run specific sub-dir. Because the baseline feature needs to be able to find the file later. It would be impractical to search all sub-dirs for the latest baseline file. |
|
@richardwerkman yes but we don't have the 'root' available in core because it's computed in Stryker cli that's why I'm considering moving the computing back up to core |
691047a to
644eba2
Compare
|
@mpownby Thanks for your patience and allowing us to think about this We talked it over and I'd still like to avoid a new parameter for this. I also don't think moving the output directory computation back to Core is the way to go. The CLI is the correct location. I consider it a bug that the disk baseline ignores --output and isn't covered by the gitignore. I propose keeping with my initial suggestion: make the disk baseline follow the output path, but make sure that location is gitignored. As you said the default output path is per-run timestamped, so the baseline can't live there or it'd never be found next run. My suggestion is to introduce the notion of a stable "output root" with the StrykerOutput directory before the timestamp and place the gitignore here. Also compute the BaselineOutput based on the StrykerOutput directory in Stryker.CLI and pass it as a new input Basically almost identical to your current change except without the new param. This would technically be a breaking change as the baseline output is now covered by a gitignore and would follow --output but functionally probably what people want. WDYT? |
(just got back from vacation, apologies for the delay) I'll give implementation a go. Are you saying that you want the baseline to appear one level up from the timestamp directory (so the timestamp directory would be a child within the output directory instead of being the output directory) ? UPDATE: I misspoke in my above comment and I think I understand what you mean now. If --output is specified, the baseline folder will be put in the output directory, as a sibling of 'reports' and .gitignore . And if it's not specified, a StrykerOutput directory will be created which will contain the .gitignore, the baesline folder and a timestamp folder which will contain the reports folder. |
83df86a to
2e4bd0f
Compare
Yes correct, so make Baseline follow the output param and move the gitignore up one level so it covers everything in output. Stryker.Core should no longer try to compute the BaselineDirectory, it should take this verbatim from the input as computed by Stryker.CLI. I think you were already most of the way there with the param implementation. I think it's fine to always place the gitignore even if output is specified by the user. If they user wants to include some files in the output directory into git they can modify the gitignore afterwards since we only place it if the file doesn't exist. |
211450b to
3abc1a0
Compare
| { | ||
| var reportPath = FilePathUtils.NormalizePathSeparators( | ||
| Path.Combine(_options.ProjectPath, _outputPath, version, "stryker-report.json")); | ||
| Path.Combine(_options.ProjectPath, _options.BaselineOutputPath, version, "stryker-report.json")); |
There was a problem hiding this comment.
Not sure if we should still path combine with the project path, I think BaselineOutputPath should always be a full path now. Maybe we should add a validation in the input validation to make sure it's a full path. Makes our lives easier in the rest of the code.
There was a problem hiding this comment.
ok I think I got this sorted. I ran another manual test and it still seems to be working properly.
The disk baseline provider always wrote the baseline to a hard-coded "StrykerOutput" folder under the project path. Storing the baseline inside the test project's directory is problematic: without a non-obvious workaround, Stryker flags it as a change to the test project and ignores the baseline. This PR changes disk baseline provider so --output is honored if specified. In this case, both a 'baseline' folder and .gitignore file will be put inside the specified output directory as siblings to the 'reports' folder. If --output is not specified, StrykerOutput will be created as before, but .gitignore will be generated inside StrykerOutput instead of inside the 'reports' folder, which fixes the problem of the baseline being incorrectly flagged as a change to the test project.
551a64e to
2bda27f
Compare
The disk baseline provider always writes the baseline to a hard-coded "StrykerOutput" folder under the project path. Storing the baseline inside the test project's directory is problematic: without a non-obvious workaround, Stryker flags it as a change to the test project and ignores the baseline.
The new optional --baseline-output argument (config: baseline.output) lets the baseline be stored and loaded from a configurable directory. Relative paths resolve against the project path; absolute paths are used as-is. It defaults to "StrykerOutput", so existing behavior is unchanged unless the option is supplied. Only the disk provider is affected; the Azure and S3 providers are untouched.
#3615