testing for libgap saving/loading workspaces #2833

dimpase · 2018-09-19T12:16:42Z

Currently loading does not work due to GAP issue #2832

codecov · 2018-09-19T14:23:24Z

Codecov Report

Merging #2833 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2833      +/-   ##
==========================================
+ Coverage   83.79%   83.79%   +<.01%     
==========================================
  Files         682      682              
  Lines      346236   346236              
==========================================
+ Hits       290135   290143       +8     
+ Misses      56101    56093       -8

Impacted Files	Coverage Δ
hpcgap/lib/hpc/stdtasks.g	`72.39% <0%> (+0.4%)`	⬆️
src/saveload.c	`68.96% <0%> (+0.53%)`	⬆️
src/vector.c	`95.2% <0%> (+1.27%)`	⬆️

dimpase · 2018-09-19T16:37:33Z

With #2840 the workspace loading test works - I'll polish it a bit more.

dimpase · 2018-09-20T11:54:30Z

OK, so now it's ready for reviewing. I've also split the only previously present in tst/testlibgap
test, basic.c, by @markuspf , into two files, to be able to reuse what's there in what I added.

markuspf

I definitely think we need a more viable approach to testing libgap, for example (a) script(s) in the testdirectory that is executed instead of multiple make targets (and essentially shell scripts that are hidden in the Makefile).

dimpase · 2018-09-21T07:11:22Z

Certainly, copy-pasting make targets is far from ideal. However, the workspace test is somewhat special, as we want to execute two applications in the correct order.

Perhaps we might also want to test that libgap-generated workspace loads in a "normal" GAP, and the other way around?

One way or another, the makefile rules probably would need to be adapted to become crossplatforms, taking filename suffixes into account.

markuspf · 2018-09-21T08:26:38Z

Have a look at the tst/test-compile directory: It has some scripts that run the tests; I imagine something like this is a viable approach to testing libgap without polluting Makefile.rules.

dimpase · 2018-09-21T09:44:35Z

Building would still need to be done by make. A part of the issue is that we'd like a generic rule to make an executable FOO linked against libgap and common.lo, something like:

        $(LINK) $(GAP_LDFLAGS) obj/tst/testlibgap/$(FOO).lo  obj/tst/testlibgap/common.lo  libgap.la -o test-libgap$(FOO)

However I'd rather leave it for an expert in GAP makefiles (@fingolfin ?) to implement.

Once this is there, writing a script invoking make and running tests won't be a problem.

We can also have a Makefile in tst/testlibgap/ to build tests.

dimpase · 2018-09-23T12:17:48Z

I will try putting -L ... into args to the call to GAP_Initialize() - this will allow to have just one make target for all the tests affected by this PR.

fingolfin · 2018-09-23T14:42:37Z

I already added such a build rule to this PR on friday, while you were sitting next to me. It is still there, and in use.

dimpase · 2018-09-23T17:33:21Z

You added the build rule, yes. I am talking about extending it to run tests, as well.

…

On Sun, 23 Sep 2018, 15:42 Max Horn, ***@***.***> wrote: I already added such a build rule to this PR on friday, while you were sitting next to me. It is still there, and in use. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2833 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN8HGA5QQo5FfePUorC6bLiLxP5QhHjks5ud53dgaJpZM4WwCfw> .

fingolfin · 2018-09-23T23:26:32Z

@dimpase My bad: I got a notification for your new comment on my smartphone, but it showed me the comment from before (from Friday 11 AM), due to that also being unread in my inbox; and I mistook that as being the comment you just had posted. But of course you really posted about the use of -L. Sorry :-(

fingolfin

I just noticed that this PR is still around, and hasn't really been reviewed. So here are some remarks. Perhaps @markuspf has more.

Perhaps this should be squashed into a single commit? Not sure how useful the history in this PR is at this point?

fingolfin · 2018-10-23T14:21:56Z

Makefile.rules

 tst/testlibgap/%: obj/tst/testlibgap/%.lo obj/tst/testlibgap/common.lo libgap.la
-	$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) $^ -o $@
+	$(LINK) $(GAP_LDFLAGS) $^ -o $@


Why is the QUIET_LINK dropped?

oops. Just forgot to put it back...

fingolfin · 2018-10-23T14:22:11Z

Makefile.rules

-	$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) $^ -o $@
+	$(LINK) $(GAP_LDFLAGS) $^ -o $@
+	$@ -A -l $(top_srcdir) -m 32m -q -T --nointeract > [email protected]
+	diff $(top_srcdir)/[email protected] [email protected]


I disagree with this part of the change: IMHO, the build rules for the test executables should be separate from the (PHONY) rules that run the tests.

Indeed, right now these targets are not marked as .PHONY, so I expect (but did not try) that running make testlibgap twice will not actually run any tests the second time around.

One way to do this should be to remove the above two lines here again; and instead have a for loop in testlibgap which iterates over $^ (resp. tst/testlibgap/basic tst/testlibgap/wscreate tst/testlibgap/wsload) and for each executes the above code. That way, you also automatically get the order right.

fixed in the latest commit

fingolfin · 2018-10-23T14:22:19Z

Makefile.rules

+	$@ -A -l $(top_srcdir) -m 32m -q -T --nointeract > [email protected]
+	diff $(top_srcdir)/[email protected] [email protected]
+
+# rm -f [email protected]


Remove this line?

tst/testlibgap/wsload.c

tst/testlibgap/common.h

tst/testlibgap/wscreate.c

dimpase · 2018-10-30T05:58:46Z

@fingolfin : hopefully this addresses all your concerns; (I'd squash the commits then).

ChrisJefferson · 2018-10-30T12:06:58Z

Makefile.rules

+
+# run all the tests in tst/testlibgap
+testlibgap: ${LIBGAPTESTS}
+	$(foreach v,$^,$(v) -A -l $(top_srcdir) -m 32m -q -T --nointeract >$(v).out; diff $(top_srcdir)/$(v).expect $(v).out;)


Why set a minimum workspace size?

In general this line seems very... clever. I have no idea at all what it's doing. If foreach a make thing? bash thing? While this isn't a requirement, I'd prefer this be a script in tst/testlibgap (but others might prefer it in here)

it's make's foreach. I can probably format this line better, split it, if you like...

minimum workspace size has been set in @markuspf 's original line in Makefile.rules to run the only available libgap's test.

I think reformatting this to cover multiple lines would improve the readability. I'd also drop the -m 32m -- if there was a reason for it, then it can be added back, but that time with a comment explaining it.

Other than that, this PR seems ready to me. Thank you for your persistence!

Thanks, fixed in the latest commit.

fingolfin · 2018-10-31T19:29:22Z

LGTM; perhaps do some squashing, though?

dimpase · 2018-10-31T21:31:00Z

rebasing all this properly is a bit beyond my git skills, with all these interleaving commits that creeped in while pulling from master, sorry. I get weird-looking errors, like Could not apply 11a347740b8ba89352c41d75f62a4ac177c56f6...

fingolfin · 2018-10-31T22:46:14Z

Hu, that's odd; just doing a git rebase master worked fine for me, followed by using git rebase -i to squash it.

Currently loading does not work due to GAP issue gap-system#2832

took `-L ...` out of the command line allow to build and run all the tests with just one makefile rule. tests in testlibgap to run each time testlibgap target is made also defined the list of tests and used it throughout. Added a TODO in wscreate regarding workspace file naming better formatting, remove -m option in libgap test calls

dimpase · 2018-10-31T23:34:37Z

OK, I've managed to squeeze it to 4 commits.

dimpase mentioned this pull request Sep 19, 2018

loading workspaces doesn't work with --nointeract option #2832

Closed

dimpase added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Sep 19, 2018

dimpase added gapdays2018-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2018-fall and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Sep 19, 2018

markuspf reviewed Sep 21, 2018

View reviewed changes

dimpase mentioned this pull request Sep 25, 2018

document functions in libgap-api.h, and related functions #2864

Closed

fingolfin requested changes Oct 23, 2018

View reviewed changes

dimpase closed this Oct 30, 2018

dimpase force-pushed the master branch from 5858540 to cb1f635 Compare October 30, 2018 00:17

dimpase mentioned this pull request Oct 30, 2018

Fork/master #2954

Closed

dimpase reopened this Oct 30, 2018

ChrisJefferson reviewed Oct 30, 2018

View reviewed changes

fingolfin approved these changes Oct 31, 2018

View reviewed changes

dimpase and others added 3 commits October 31, 2018 23:14

testing for libgap saving/loading workspaces

a2b733a

Currently loading does not work due to GAP issue gap-system#2832

created expected output files; added cleanup to rules

8ba82ab

buildsys: refactor testlibgap and friends

382f95c

dimpase force-pushed the master branch from 9eef01f to a6e47a1 Compare October 31, 2018 23:33

fingolfin merged commit 3c110e7 into gap-system:master Nov 1, 2018

fingolfin added topic: libgap things related to libgap topic: tests issues or PRs related to tests release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Apr 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing for libgap saving/loading workspaces #2833

testing for libgap saving/loading workspaces #2833

dimpase commented Sep 19, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

dimpase commented Sep 19, 2018

dimpase commented Sep 20, 2018

markuspf left a comment

dimpase commented Sep 21, 2018

markuspf commented Sep 21, 2018

dimpase commented Sep 21, 2018 •

edited

Loading

dimpase commented Sep 23, 2018

fingolfin commented Sep 23, 2018

dimpase commented Sep 23, 2018 via email •

edited

Loading

fingolfin commented Sep 23, 2018

fingolfin left a comment

fingolfin Oct 23, 2018

dimpase Oct 23, 2018

dimpase Oct 30, 2018

fingolfin Oct 23, 2018

dimpase Oct 30, 2018

fingolfin Oct 23, 2018

dimpase Oct 23, 2018

dimpase commented Oct 30, 2018

ChrisJefferson Oct 30, 2018

ChrisJefferson Oct 30, 2018

dimpase Oct 30, 2018

dimpase Oct 30, 2018

fingolfin Oct 31, 2018

dimpase Oct 31, 2018

fingolfin commented Oct 31, 2018

dimpase commented Oct 31, 2018

fingolfin commented Oct 31, 2018

dimpase commented Oct 31, 2018

testing for libgap saving/loading workspaces #2833

testing for libgap saving/loading workspaces #2833

Conversation

dimpase commented Sep 19, 2018

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

dimpase commented Sep 19, 2018

dimpase commented Sep 20, 2018

markuspf left a comment

Choose a reason for hiding this comment

dimpase commented Sep 21, 2018

markuspf commented Sep 21, 2018

dimpase commented Sep 21, 2018 • edited Loading

dimpase commented Sep 23, 2018

fingolfin commented Sep 23, 2018

dimpase commented Sep 23, 2018 via email • edited Loading

fingolfin commented Sep 23, 2018

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimpase commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Oct 31, 2018

dimpase commented Oct 31, 2018

fingolfin commented Oct 31, 2018

dimpase commented Oct 31, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

dimpase commented Sep 21, 2018 •

edited

Loading

dimpase commented Sep 23, 2018 via email •

edited

Loading