Document baseline update and allow tests to run in debug mode without assert #8149
Conversation
dsyme
commented
Jan 9, 2020
•
dsyme
commented
- document how to update test baselines in bulk
- allow tests to run in debug mode without assert (the assert is spurious an incorrect)
- allow perl to run without perl installed on machine in path
DEVGUIDE.md
Outdated
| ``` | ||
| set TEST_UPDATE_BSL=1 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Should a link to https://github.com/dotnet/fsharp/tree/master/tests/fsharp#workflow-when-adding-or-fixing-tests be added as I gather this environment variable only affects fsharpqa tests and running from command line, while the section I'm pointing at is handling the other fsharp tests (which are generally run from the IDE or a test runner).
For context, the reason I made the directory list explicit in the update.base.line.with.actuals.fsx script is that it requires more attention when updating the baselines, always fearing something goes wrong with "en masse" updating depending the scenario.
Maybe a word of caution about doing "en masse" update of all baselines to be used mostly with a message change but not when code / logic in the compiler is changed as well which requires more careful review.
There was a problem hiding this comment.
Baselines should be updated carefully. Having a tool to batch update them makes updating baselines easier, but it may introduce bugs that no one notices due to incorrect baseline updates.
@dsyme can we make this a bit more surgical? perhaps a script with a specific path to the test case to update.
There was a problem hiding this comment.
I'll add that caution to the note. But the best place to be vigilant about this is in code review - mass update of baselines should correspond to minimal compiler changes where possible.
There was a problem hiding this comment.
Have added this:
Updating baselines en-masse should be done very carefully and subject to careful code review. Where possible the
compiler change causing the en-masse update should be isolated and minimized so it is obvious at review time that no other code generation chagnes will be caused.
I'm just documenting what's there for now, not seeking to improve it :)
There was a problem hiding this comment.
@smoothdeveloper We should probably just combine https://github.com/dotnet/fsharp/blob/master/tests/fsharp/readme.md into DEVGUIDE.md - I'd actually forgotten that file was there. I guess it's the right place for this note though, I'll move it
There was a problem hiding this comment.
@dsyme, possibly, although I like the idea of having readme specific to folders, they show up when you browse the repository from github, making it more user friendly to people perusing in the repository, and it is clear that the contents apply to that part of the tree only.
A very long DEVGUIDE.md with lots of details about specific parts of the repository may be less inviting and lead to overlook the top most information.
I rather have a DEVGUIDE.md being an entry point and geared toward general development, and linking to specific areas rather than an ever expanding file containing every specific development use case, this should scale better.
It is subjective which one is best anyways but I hope it clarifies why the folder specific files came.
In any case, thanks for adding the "be careful" notice, I think that was the most important!
| @@ -5089,7 +5089,6 @@ and GenDecisionTreeSwitch cenv cgbuf inplabOpt stackAtTargets eenv e cases defau | |||
| | _ -> error(InternalError("these matches should never be needed", switchm)) | |||
|
|
|||
| and GenDecisionTreeCases cenv cgbuf stackAtTargets eenv defaultTargetOpt targets repeatSP targetInfos sequel caseLabels cases (contf: Zmap<_,_> -> FakeUnit) = | |||
| assert(cgbuf.GetCurrentStack() = stackAtTargets) // cgbuf stack should be unchanged over tests. [bug://1750]. | |||
There was a problem hiding this comment.
Why is this an incorrect assert? I assume based on the comment that it is very old
There was a problem hiding this comment.
My understanding is it doesn't properly take into some logic associated with "unit" values - the two views of the stack are different.
|
Something in this PR seems to have broken the "release/fsharp5" branch when it was integrated across. I'll see if I can identify what See https://dev.azure.com/dnceng/public/_build?definitionId=496&_a=summary&view=branches for branch status |
| @@ -147,7 +147,7 @@ | |||
| my $FILE_ERROR_EXITVAL = 5; | |||
| my $OTHER_ERROR_EXITVAL = 9; | |||
|
|
|||
| my $perl = $Config{perlpath}; | |||
| my $perl = $^X; | |||
There was a problem hiding this comment.
either it was this change or the assertion removal that borked things
… assert (dotnet#8149) * document baseline update and allow tests to run in debug mode without assert * Update DEVGUIDE.md * Update DEVGUIDE.md * Update readme.md
