[WIP] Support run GHC's test from hadrian.

[sighingnow]

1. Necessary command line arguments to run test driver.
   • --test-only=<TESTS>
   • --skip-perf
   • --summary=<TEST_SUMMARY>
   • --junit=<TEST_SUMMARY_JUNIT>
   • --config=<EXTRA_TEST_CONFIG>
2. Synchronize configurations from test.mk.
3. Synchronize GHC's compilation flags from test.mk (that's very important).

WIP

• [x] I'm not sure if it's fine to add these command line arguments for running test.
• [ ] Document about how to use hadrian to run one or more test cases.

[sighingnow]

Another question is that I have noticed that currently hadrian just call sytem's make to run the validate target. Should we move to make to run test target or call the runtest.py script from hadrian directly ?

[snowleopard]

@sighingnow Many thanks for this. To answer your questions:

I'm not sure if it's fine to add these command line arguments for running test.

I think that's fine. Otherwise there is really no way to conveniently use the built-in test rule, because one often wants to test just a single test case. However, I think we don't need the test- prefixes, because that would lead to a bit of duplication in command lines, e.g. hadrian test --test-only=123. Let's try to keep the flags exactly as they are currently in the driver script, e.g. hadrian test --only=123. This also has the benefit that GHC developers do not need to learn any new flags.

Another question is that I have noticed that currently hadrian just call sytem's make to run the validate target. Should we move to make to run test target or call the runtest.py script from hadrian directly ?

At the moment we run validate via make because that's the simplest thing to do. However, in future I think it would be best to get rid of make completely, and possibly even rewrite the runtest logic in Hadrian directly. Does this answer your question?

[sighingnow]

Thanks for your reply and review. I will fix these problem this weekends. I'll work on the validate rule after finishing the test rule.

[alpmestan]

I recently got started tackling the very same thing, but you went through much more trouble to integrate this properly with command-line flags etc. Can this run .T tests properly too, or more generally can we just run all tests and get the same result that we get with the make build system? Some tests have Makefiles that rely on some env vars being set etc.

I got started looking into this in the context of https://github.com/snowleopard/hadrian/pull/445 but if you do most of the work, that would be great and I'll then just have to tweak a couple of things to make it work with that branch. So... thanks! :)

[simonmar]

@alpmestan aren't you also working on this? cc @bgamari

[alpmestan]

@simonmar I am indeed, I left a comment above to ask a question or two. This patch is most likely saving me quite some work.

[snowleopard]

@alpmestan Ah, hopefully there wasn't too much work duplication. Note that this PR grew out of solving a bug in the test rule: #494. Perhaps, we should create a more general issue to coordinate the work on test build rules in Hadrian.

[alpmestan]

@snowleopard Well if this PR gets us to a point where we can run tests with the same results that we get with the make build system, then I don't think there's much more to do for now, hence my questions above :)

I haven't done a lot of duplicate work, but I did some. It's not all lost though, as this allowed me to look at and understand the testing infra of GHC as well as more code in Hadrian.

[sighingnow]

@alpmestan Currently I just have taken flags from test.mk to the test rule to execute the python script.

Make hadrian's test and validate rules fully support GHC's testsuite needs working with GHC team to translate the Makefile script in some tests to hadrian/shake's rule. I have run hadrian validate and many testcases failed to run their Makefile scripts. test summary log

I'm willing to take the job to integrate hadrian to GHC's test and validate.

[alpmestan]

@sighingnow I see! So we are both facing the same problem. One idea that I had yesterday to handle this problem without touching GHC's codebase at all might do the trick but requires going through a few hacky hoops.

What if we copied over the makefiles for all the tests that have one, along with the files needed for the test, and put them somewhere under the build root while following the same hierarchy. Except that we could edit the makefiles on the fly. As far as I can tell, many (most? all?) makefiles define TOP, include boilerplate.mk and test.mk and then define rules for the tests, which end up running simple commands like $(TEST_HC) $(TEST_HC_OPTS) [...] foo.hs -o foo. What if we simply stripped of the problematic bits (TOP and the includes) and instead defined TEST_HC, TEST_HC_OPTS to point to the GHC we've built and our config etc? I don't know how many such variables have to be defined, but I was looking around yesterday and I've really just seen these 2. I haven't looked everywhere though of course.

It sounds to me like such an approach could work and has the huge advantage that this doesn't require a single change to GHC's codebase. I'm not sure defining TEST_HC and what not on the spot is the best option, but I do think it is necessary to work with variations on those Makefiles that don't rely on half of the make based build system, therefore the suggestion to strip off the includes. If we don't do this, then we don't have to just hook into the python driver correctly but also into the make build system, and I suspect this is something we want to avoid as much as possible, or even entirely.

[sighingnow]

Yeah, we just need to remove the include in Makefile scripts and do some tricks in python's run_command to set necessary variables, for example, $TOP.

We could leave the .T file as it is and only modify the Makefile in every test cases (remove these include lines).

[alpmestan]

Yeah, we just need to remove the include in Makefile scripts and do some tricks in python's run_command to set necessary variables.

Either that or define those variables in our modified Makefiles, or in the environment in which the python script is executed by Hadrian, if we want to leave GHC's testsuite driver code untouched (which would be a good thing IMO, but it's absolutely not necessary).

We could leave the .T file as it is and only modify the Makefile in every test cases.

Right, but I think the makefiles rely on having the .hs (and others) files in the same directory, so we would probably have to move those over as well, without changing them though.

Alright @sighingnow, are you willing to give a shot at implementing this or should I?

[sighingnow]

I still don't think we need to move these files, before run_command the python driver has already changed to the right directory. For example:

cd "testsuite/tests/printer/Ppr017.run" && $MAKE -s --no-print-directory ppr017

are you willing to give a shot at implementing this or should I?

Do you mean modify the test cases' Makefile and hack the environment variables before run test ?

I think we can pass necessary variables via system's environment variable. Define these variables in Makefile is not a good idea since we may want to run test with another ghc instance.

[sighingnow]

are you willing to give a shot at implementing this or should I?

@alpmestan I'm really not familiar with hadrian's codebase so it would be better if you could submit such a patch :)

I'll try to follow your work.

[alpmestan]

Do you mean modify the test cases' Makefile and hack the environment variables before run test ?

Well, anything that gets all the tests to be executed properly, really :) If you want to implement a plan you have in mind, that's fine too!

[alpmestan]

Just to clarify: I'm by no means an expert on Hadrian's code. I've just been working on/with it for several few weeks. I took the liberty to give some feedback and suggestions on ways to fix the test rules because I'm myself facing the same problem. I'm nowhere saying my suggestions are better, just trying to figure out a way to get hadrian to be a fine companion for running tests.

[snowleopard]

I'm really not familiar with hadrian's codebase I'm by no means an expert on Hadrian's code

@sighingnow @alpmestan I'm happy to be your go-to expert on Hadrian :) Feel free to submit half-cooked PRs for review and I'll suggest how to make the best use of Hadrian infrastructure and/or do some polishing after merging. Thank you for helping!

[alpmestan]

@snowleopard For the record, @sighingnow and I had a little chat by email, and he's willing to take a stab at implementing something along the lines of what we have been suggesting above. By combining everyone's knowledge, fixes and experiments, we will get that testsuite running smoothly sooner or later :)

(While you're around Andrey, I think it might be good that you and (Moritz and/or me) have a little discussion about https://github.com/snowleopard/hadrian/pull/445 and how we should go about merging most/all of that work -- should I start an email thread? Sorry for the off-topic question on this PR.)

[snowleopard]

@alpmestan Awesome!

Sure, feel free to start an email thread to discuss #445. There is already a bit of a discussion in the PR itself, so let's include all participants. We could also setup a Skype chat to speed things up.

[sighingnow]

Update summary:

1. Move TestArg definition to CommandLine.hs.
2. Add runTestBuilderArgs under the Settings/Builders directory to collect all necessary arguments to run python driver.
3. Add timeout program target in testRules.
4. Set environment variables TEST_HS and TEST_HC_OPTS before run python script, Now the tests which use Makefile script can work after deleting include lines in their Makefile, (for example, T2578 and T14626 under codeGen/should_compile), without touching or moving the .T file.

Help wanted:

1. I'm really not sure if I do the right things in runTestBuilderArgs in the newly created RunTest.hs.
2. Currently I need to compute ghcFlags twice, in runTestBuilderArgs and testRules. I need to pass these flags to python driver, as well as set these flags to TEST_HC_OPTS environment variables. How could I avoid the boilerplate code ?

/cc @snowleopard @alpmestan

[alpmestan]

Regarding question number 1: the code so far looks reasonable. It is much nicer to have a proper "builder" for running the tests. If I were you I would not worry much more about that code at least until the tests all run properly. :)

Regarding question number 2, I think @snowleopard or someone else will have a better answer.

[snowleopard]

I'm really not sure if I do the right things in runTestBuilderArgs in the newly created RunTest.hs.

@sighingnow This looks good to me.

Currently I need to compute ghcFlags twice

Can you simply export ghcFlags from Settings.Builders.RunTest? That's what I would do. I hope that doesn't lead to an import cycle.

Great job! I've left a few other minor comments above. In general, I think you sometimes could shorten the code by using the operator ? for adding certain command line flags conditionally.

[sighingnow]

Can you simply export ghcFlags from Settings.Builders.RunTest? That's what I would do. I hope that doesn't lead to an import cycle.

Now I define ghcFlags in Rules.Test and import it in Settings.Builders.RunTest to make it less boilerplate.

[snowleopard]

@sighingnow Many thanks! I think we are getting close to merging this. My apologies for slow responses, I am travelling this week.

[snowleopard]

@sighingnow I've left a few more comments.

At the top of the PR one of your tasks is to add docs describing how to run tests. Are you going to add these docs as part of this PR? Or do you prefer to do it in a separate PR?

Thank you!

[sighingnow]

I have read the task list in #187 . I prefer to add doc after #187 get fixed.

Many TODOS still exists in this PR. I need more time to understanding other stuffs in GHC's test script, such as --way=... and finish hadrain's 'test' and validate rules.

[sighingnow]

I suggest to use the Squash and merge option when merge this PR to make a clean diff for others who also is interested in this work. Thanks!

@snowleopard

[snowleopard]

@sighingnow Many thanks, I've squashed & merged this. Looking forward to seeing more PRs from you :)