Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document baseline update and allow tests to run in debug mode without assert #8149

Merged
merged 4 commits into from
Jan 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/fsharp/IlxGen.fs
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this an incorrect assert? I assume based on the comment that it is very old

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is it doesn't properly take into some logic associated with "unit" values - the two views of the stack are different.


match defaultTargetOpt with
| Some defaultTarget ->
Expand Down
14 changes: 14 additions & 0 deletions tests/fsharpqa/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,17 @@ A convenience "fsharpqafiles.csproj" project is located in the fsharp.sln soluti

(TODO, provide some guidance about how to define env.lst files)

## Updating baselines in tests

Some tests use "baseline" files. There is sometimes a way to update these baselines en-masse in your local build,
useful when some change affects many baselines. For example, in the 'fsharpqa' tests the baselines
are updated using scripts or utilities that allow the following environment variable to be set:

```
set TEST_UPDATE_BSL=1
```

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.

2 changes: 1 addition & 1 deletion tests/fsharpqa/testenv/bin/runall.pl
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@
my $FILE_ERROR_EXITVAL = 5;
my $OTHER_ERROR_EXITVAL = 9;

my $perl = $Config{perlpath};
my $perl = $^X;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either it was this change or the assertion removal that borked things


BEGIN {
@required_mods = ("Win32\\Process.pm");
Expand Down