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

Allow local variables in test files #1633

Merged
merged 5 commits into from
Nov 15, 2018

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Aug 25, 2017

This is an experiment in allowing local variables to be declared in test files.

The current (horrible) notation is to write:

#@local <list of variables to be local> as the first line of the test. If this line is present, any other introduced variables will produce a warning, as if we were in a function.

There are two reasons to do this:

  1. You don't pollute the global name space, local names do not exist at the end of the test.

  2. (More importantly) you can run the test in two threads simultaniously, as the two local contexts will not interfere.

This will fail, to demonstrate the new warnings being produced.

UPDATEUPDATE:

This PR also now features #@if, #@else, #@endif, which provides an easy way to skip parts of a given test, or give different returns for a given test. This is now used to make the 32-bit, 64-bit and unix tests only run on the correct platforms.

@ChrisJefferson ChrisJefferson added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Aug 25, 2017
@codecov
Copy link

codecov bot commented Aug 28, 2017

Codecov Report

Merging #1633 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1633      +/-   ##
==========================================
+ Coverage   85.13%   85.15%   +0.02%     
==========================================
  Files         110      110              
  Lines       56964    56964              
==========================================
+ Hits        48494    48506      +12     
+ Misses       8470     8458      -12
Impacted Files Coverage Δ
src/vars.c 92.69% <100%> (ø) ⬆️
src/streams.c 71.06% <100%> (+0.12%) ⬆️
src/stats.c 95.07% <0%> (-0.21%) ⬇️
src/exprs.c 97.37% <0%> (-0.18%) ⬇️
src/gap.c 83.81% <0%> (-0.1%) ⬇️
src/intrprtr.c 98.88% <0%> (+0.22%) ⬆️
src/read.c 97.01% <0%> (+0.72%) ⬆️
src/iostream.c 63.49% <0%> (+1.14%) ⬆️

@olexandr-konovalov
Copy link
Member

@ChrisJefferson you've asked for further suggestions - what about making it possible to specify whether the test file or a separate example should just run without entering a break loop or crashing (so diffs are permitted)?

@ChrisJefferson
Copy link
Contributor Author

We could certainly do that

@fingolfin
Copy link
Member

I wonder if we can also write a tool which automatically generates such a "local" line for an existing .tst file. That would greatly help in converting existing test files.
The idea is that in this mode, instead of reporting undefined locals, we add them to a list (and swallow the warning); then at the end of the .tst file, use that list to generate the comment. Perhaps we can even add them to the LVAR on the fly?

Of course for this won't work perfect, because a few tests may want to define a global (to test it), but those should be rare exceptions.

@ChrisJefferson
Copy link
Contributor Author

I'm sure we could -- there is a "nice" way of doing that (hooking into the access to undefined variable, and recording when it occurs), and a "nasty" way (just look through the output for the error we know we will see), and either way stick a new local line at the top of the file.

src/vars.c Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

Perhaps rebase and squash the fixup commits?

@ChrisJefferson ChrisJefferson force-pushed the local_test branch 4 times, most recently from c2343fd to f6a3b35 Compare October 23, 2017 10:29
@olexandr-konovalov
Copy link
Member

@ChrisJefferson ping?

lib/test.gi Outdated Show resolved Hide resolved
@ChrisJefferson ChrisJefferson force-pushed the local_test branch 7 times, most recently from 40f0acf to df2ee3f Compare July 5, 2018 19:21
@ChrisJefferson ChrisJefferson removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jul 5, 2018
@ChrisJefferson ChrisJefferson force-pushed the local_test branch 2 times, most recently from 8eb2ea3 to bed4eac Compare November 6, 2018 09:43
@ChrisJefferson
Copy link
Contributor Author

Back from the depths of time, I believe all issues are fixed and this PR is ready to merge.

tst/testinstall/randlist.tst Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
lib/test.gi Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

All changes made (I think), except the #@exec -> #@Local, which I'm happy to discuss.

@fingolfin
Copy link
Member

When you write #@exec -> #@Local, do you mean #@exec -> #@global? Otherwise, I don't follow...

In case you do mean that: Well, in the end, I can certainly accept either; and I don't want to hold up the PR with bikeshedding; it's just that once a feature is out there, it is difficult to take it back. Since #@global would be much more limited in scope, I think it'd be very easy to document and easy to understand and use correctly; whereas I am afraid that I don't fully understand all potential ramifications of #@exec, and so I worry about abuse... Like, one thing I am concerned about is that apparently all #@exec in the whole .tst file are collected and executed at once before everything else in the script. So I could add #@exec lines at the bottom, and they'd be executed first thing, which I find highly surprising. I guess I'd be less concerned if @#exec was executed in lock step with other commands, i.e., only at the point they appear in the file. Or alternatively, if they were restricted to only appear at the top of the file, before any non-comment non-empty line.

OTOH, the added power of #@exec might very well be a boon in ways I cannot foresee yet...

Would be interesting to know what others thinks, too? E.g. @markuspf @stevelinton @wilfwilson etc.

@ChrisJefferson
Copy link
Contributor Author

One problem with #@global is that we wouldn't be able to define the value the variable has (we would have to choose a default, 0 perhaps). If we let people write a value (by parsing/splitting #@global x := value for example, then we are back to having to either execute the whole statement, or reading and evaluating value, both of which don't really limit the power.

@ChrisJefferson
Copy link
Contributor Author

This isn't documented, but both #@Local and #@exec must occur before any gap statements (this wasn't particularly planned, but it is how the code works and seems like a sensible restriction).

@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Nov 7, 2018

I've added a brief bit to the docs saying #@Local and #@exec must come before any tests.

@fingolfin
Copy link
Member

BTW, is there a reason you keep writing #@Local (with capital L) in comments and commit messages, while it is #@Local in the actual code and tests (and you seem to never capitalize #@exec)

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Now that you clarified that #@exec can only be used at the top, I am happy enough :-)

@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Nov 7, 2018

It's github #@Local #@exec are both lower-case when I write them -- github is linking them to the Local organisation (which is capitalised) and the exec organisation (which isn't)

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

I've had a little look, I like the new features, and I don't have any objections.

@markuspf markuspf added kind: new feature topic: tests issues or PRs related to tests release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Nov 14, 2018
@markuspf
Copy link
Member

Could you rebase again so we can merge this?

@markuspf markuspf merged commit 92b1d16 into gap-system:master Nov 15, 2018
@ChrisJefferson ChrisJefferson deleted the local_test branch January 20, 2019 13:22
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 15, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: new feature release notes: added PRs introducing changes that have since been mentioned in the release notes topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants