Added support for testsuite #602

chitrak7 · 2018-05-13T19:34:36Z

@snowleopard @angerman
Validation result after this push:
SUMMARY for test run started at Mon May 14 00:32:42 2018 IST
0:20:37 spent to go through
6359 total tests, which gave rise to
23523 test cases, of which
17231 were skipped

  42 had missing libraries
6098 expected passes
  94 expected failures

   0 caused framework failures
   0 caused framework warnings
   1 unexpected passes
  55 unexpected failures
   2 unexpected stat failures

snowleopard · 2018-05-13T21:33:40Z

src/Rules/Test.hs

+    f :: Stage -> Package -> Action FilePath
+    f stage pkg | isLibrary pkg = pkgConfFile (Context stage pkg (read "v"))
+                | otherwise     = programPath =<< programContext stage pkg
+
 needTestBuilders :: Action ()
 needTestBuilders = do
    needBuilder $ Ghc CompileHs Stage2
    needBuilder $ GhcPkg Update Stage1
    needBuilder Hp2Ps


Hp2Ps is already included into testsuitePackages, so I guess we can drop this line?

snowleopard · 2018-05-13T21:35:32Z

src/Settings/Builders/Make.hs

+    check_ppr             <- f checkPpr
+    check_api_annotations <- f checkApiAnnotations    
+    return ["TEST_HC=" ++ test_hc, "CHECK_PPR=" ++ check_ppr, "CHECK_API_ANNOTATIONS=" ++ check_api_annotations]
+  where


Let's rename f to something more informative, perhaps, fullPath.

Also, a type signature might help.

snowleopard · 2018-05-13T21:37:41Z

src/Rules/Test.hs

+    need targets
+  where
+    f :: Stage -> Package -> Action FilePath
+    f stage pkg | isLibrary pkg = pkgConfFile (Context stage pkg (read "v"))


I think you need vanillaContext here.

Btw, are there testsuite-only libraries? If yes, are you sure vanilla is sufficient for the testsuite?

This bit of code is taken directly from Rules.hs.
There are many errors coming from rts library (40). Although I still couldn't figure out from where errors are coming, I have added this part just in case.

snowleopard · 2018-05-13T21:39:48Z

src/Rules/Test.hs

+    targets <- mapM (f Stage1) =<< testsuitePackages
+    need targets
+  where
+    f :: Stage -> Package -> Action FilePath


Please choose a more informative name instead of f.

snowleopard · 2018-05-13T21:41:01Z

src/Rules/Test.hs

@@ -63,13 +64,24 @@ testRules = do
        -- Execute the test target.
        buildWithCmdOptions env $ target (vanillaContext Stage2 compiler) RunTest [] []

+-- | Build extra programs required by testsuite


The code below suggests that there are not only testsuite programs, but also testsuite libraries.

@chitrak7 So, do you really need to handle libraries here?

I have to handle rts library. I just cannot yet figure out how.

Why do you need to handle rts here? How is it related to the testsuite?

I still don't understand why needTestsuiteBuilders needs libraries. Can testsuitePackages contain library packages? Right now there are only contain programs. If testsuitePackages can contain libraries, then you need to rename this function to something like needTestsuitePackages (instead of Builders).

Some files are missing in rts library causing multiple failures. At the moment, I cannot figure out which files are missing. Or they may be missing includes. I have to look into this matter further.

I don't understand how rts relates to this question. You don't have rts in testsuitePackages.

The comment for this function says "Build extra programs required by testsuite", but the implementation can also build libraries. This is an inconsistency that should be fixed.

Okay! I think I should create an issue regarding this and temporarily install required programs only.

snowleopard · 2018-05-13T21:41:27Z

@chitrak7 Thanks! I've added some comments.

alpmestan · 2018-05-14T09:19:45Z

@chitrak7 Do you have any idea at this point of why we're still skipping so many tests? (this is not a comment/request on the actual patch, I'm just curious and this still feels like the best place to ask)

izgzhen · 2018-05-14T16:18:41Z

src/GHC.hs

+    hsc2hs, hp2ps, hpc, hpcBin, integerGmp, integerSimple, iservBin, libffi, 
+    mtl, parsec, parallel, pretty, primitive, process, rts, runGhc, stm,
+    templateHaskell, terminfo, text, time, touchy, transformers, unlit, unix,
+    win32, xhtml, ghcPackages, isGhcPackage, defaultPackages, testsuitePackages,


nit: this diff looks unnecessarily big

@izgzhen At the moment this is unavoidable. We might refactor this to one package per line at some point if such large diffs look distracting, but it would be better to do it in a separate PR.

chitrak7 · 2018-05-15T16:17:55Z

@alpmestan I do not exactly know that, Can you give me pointers on where to look. More importantly, why does testsuite decide to skip a test.

chitrak7 · 2018-05-15T17:31:15Z

@snowleopard Can you please see why recursive rule failure is coming?

alpmestan · 2018-05-15T18:10:48Z

@alpmestan I do not exactly know that, Can you give me pointers on where to look. More importantly, why does testsuite decide to skip a test.

You should see some information about this in the testsuite driver initialization part of the output (early on, when you look at the full log).

chitrak7 · 2018-05-15T21:35:11Z

@alpmestan I believe skipped tests are because of using "fast" option with testsuite.

alpmestan · 2018-05-15T21:42:13Z

@chitrak7 OK. If you ever get a chance to, it would be terrific to add a way to run the equivalent of ./validate --slow. If not, I'll do it. But not in this PR of course. Thanks for the reply and taking a look at what was going on 👍

snowleopard · 2018-05-18T08:50:29Z

@chitrak7 Please resolve merge conflicts.

snowleopard · 2018-05-18T09:06:28Z

Can you please see why recursive rule failure is coming?

@chitrak7 The recursive rule failure is unrelated to your PR. I think the reason is #599.

snowleopard · 2018-05-18T22:17:21Z

@chitrak7 Something is wrong, looks like you've duplicated the recent commits instead of rebasing.

chitrak7 · 2018-05-19T12:12:03Z

@snowleopard I have reset my repository and made necessary changes. It is currently failing due to "llvm" issue. I will resend it once that PR Lands.

sighingnow · 2018-05-23T01:53:59Z

Should the title of this PR be "Add support for nofib testsuite" ? We already support hadrian test.

alpmestan · 2018-05-23T09:02:19Z

No, support for nofib was the goal of #599, not this PR. This PR is about supporting the execution of more tests, and about building things that are required by some of them which were not built previously. It still skips a whole bunch of tests but maybe @chitrak7 could provide an overview of the state of things with/without the commits from his branch? In terms of # of tests being run, # of broken test expectations.

sighingnow · 2018-05-23T09:11:03Z

I just noticed that this PR also add a new file src/Rules/Nofib.hs...

alpmestan · 2018-05-23T10:05:39Z

Right, my commits somehow did not get merged properly apparently. But you cann tell they've already been merged because they refer to the PR number.

chitrak7 · 2018-06-04T18:57:22Z

Hi @snowleopard @angerman, I have pushed the work done with validation implementation.

snowleopard · 2018-06-04T21:59:20Z

@chitrak7 Thanks! Let's come back to this PR once we fix Hadrian.

snowleopard · 2018-06-12T15:30:56Z

src/Hadrian/Utilities.hs

@@ -481,3 +484,6 @@ renderUnicorn ls =
    ponyPadding = "                                            "
    boxLines :: [String]
    boxLines = ["", "", ""] ++ (lines . renderBox $ ls)
+
+data TestSpeed = Slow | Average | Fast deriving (Show, Eq)


I'm not sure TestSpeed is a generally useful data type. It's certainly useful for the GHC build system, but other build systems may have different settings or may not even need a testsuite (remember, Hadrian.* namespace is for generic functionality that may be useful not only for building GHC).

I suggest to move TestSpeed to the CommandLine module.

snowleopard · 2018-06-12T15:36:24Z

src/Settings/Builders/Make.hs

+    compiler            <- expr $ fullpath ghc
+    checkPpr            <- expr $ fullpath checkPpr
+    checkApiAnnotations <- expr $ fullpath checkApiAnnotations
+    let t = show $ max 4 (threads - 2)


Since the validation script will likely be the only running thread, we do not need to reduce parallelism like we do with other Make invocations. I suggest we use the threads settings as is.

snowleopard · 2018-06-12T15:41:08Z

src/CommandLine.hs


+readTestWays :: Maybe String -> Either String (CommandLineArgs -> CommandLineArgs)
+readTestWays ways = 


I don't understand, does this argument correspond to (multiple) ways or to a single way? Looking below, you expect to see a single way in Just. Shall this be renamed? If you worry about the clash of names, you can use maybeWay here.

Every argument takes a single way. Multiple ways can be specified by this taking one way at a time.
eg: --test-way=way1 --test-way=way2

Then this function should be renamed to readTestWay, since it reads a single way.

snowleopard · 2018-06-12T15:42:49Z

src/CommandLine.hs

+    , Option [] ["test-verbose"] (OptArg readTestVerbose "TEST_VERBOSE")
+      "A verbosity value between 0 and 5. 0 is silent, 4 and higher activates extra output."
+    , Option [] ["test-way"] (OptArg readTestWays "TEST_WAY")
+      "only run these ways" ]


We need to document all these flags. You don't need to do this right in this PR, but at least please create an issue so that we don't forget about this.

sighingnow · 2018-06-12T16:43:49Z

src/CommandLine.hs

-    , testJUnit    :: Maybe FilePath
-    , testConfigs  :: [String] }
+    , testThreads  :: Maybe String
+    , testVerbosity:: Maybe String


Hadrian (or Shake) already has -jN option, which is enough to control the speed of running testsuite. We don't need the testThreads here.

@sighingnow Indeed! Well spotted.

Removed indentation error

snowleopard · 2018-06-12T17:16:47Z

src/Hadrian/Utilities.hs

@@ -30,7 +30,7 @@ module Hadrian.Utilities (
    (<&>), (%%>), cmdLineLengthLimit,

    -- * Useful re-exports
-    Dynamic, fromDynamic, toDyn, TypeRep, typeOf
+    Dynamic, fromDynamic, toDyn, TypeRep, typeOf,


Drop the comma

snowleopard · 2018-06-12T17:20:13Z

src/Settings/Builders/Make.hs

+    compiler            <- expr $ fullpath ghc
+    checkPpr            <- expr $ fullpath checkPpr
+    checkApiAnnotations <- expr $ fullpath checkApiAnnotations
+    return [ "fast"


Is the "fast" above related to TestSpeed.Fast?

No, this is the default validate argument. Speed can be specified by running ./build.sh test and not validate.

Worst case, we can later arrange to support --slow and --fast flags, like the actual validate script, can't we?

Yeah. I can do that right now.

OK, I guess we can leave this for later.

snowleopard · 2018-06-12T19:23:58Z

OK, I think we can merge this -- there are a few minor issues but we can fix them later.

@alpmestan Do you have any further comments?

alpmestan · 2018-06-12T20:26:32Z

No, let's merge 👍

sighingnow · 2018-06-13T08:13:26Z

src/CommandLine.hs

    , testSummary  :: Maybe FilePath
-    , testJUnit    :: Maybe FilePath
-    , testConfigs  :: [String] }
+    , testVerbosity:: Maybe String


Shake already provides a --verbose option, let's reuse it.

The verbose option of Shake is boolean, whereas test suite supports 5 levels of verbosity. This is why I chose to pass a separate verbose argument.

I think that it may be convenient to control these two verbosity settings separately: you might want verbose testing, but not Shake diagnostic info. And vice versa. So, let's keep this flag.

snowleopard · 2018-06-13T10:29:10Z

Merged. Many thanks @chitrak7, and everyone else for the review!

snowleopard reviewed May 13, 2018

View reviewed changes

izgzhen reviewed May 14, 2018

View reviewed changes

chitrak7 force-pushed the master branch 2 times, most recently from 4a04db1 to b439d95 Compare June 4, 2018 18:53

snowleopard reviewed Jun 12, 2018

View reviewed changes

sighingnow suggested changes Jun 12, 2018

View reviewed changes

chitrak7 added 10 commits June 12, 2018 22:18

Rule for testsuite dependencies

39e3cc2

Separated validate builder arguments

0d6050b

Added RunTest config options

fffd876

added support to set test speed with runtest

a6b5e05

Fixed minor bug with testConfigs

cf67794

Removed indentation error

Added support for more testing features

b98524f

Rectified Merge Errors

d06ef68

Removed need rule for Hp2ps

5961be4

using all available threads

29a6a0d

Minor Revision

d886a74

chitrak7 force-pushed the master branch from 952e7d7 to d886a74 Compare June 12, 2018 16:58

Removed TestThread argument

cf9a443

snowleopard reviewed Jun 12, 2018

View reviewed changes

Update Utilities.hs

71d87c8

sighingnow suggested changes Jun 13, 2018

View reviewed changes

sighingnow approved these changes Jun 13, 2018

View reviewed changes

snowleopard merged commit ef4137d into snowleopard:master Jun 13, 2018

chitrak7 mentioned this pull request Jun 15, 2018

Add validation support #187

Closed

8 tasks


		readTestWays :: Maybe String -> Either String (CommandLineArgs -> CommandLineArgs)
		readTestWays ways =

Added support for testsuite #602

Added support for testsuite #602

Conversation

chitrak7 commented May 13, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard Jun 12, 2018 • edited Loading

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

snowleopard commented May 13, 2018

alpmestan commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chitrak7 commented May 15, 2018

chitrak7 commented May 15, 2018

alpmestan commented May 15, 2018

chitrak7 commented May 15, 2018

alpmestan commented May 15, 2018

snowleopard commented May 18, 2018

snowleopard commented May 18, 2018

snowleopard commented May 18, 2018

chitrak7 commented May 19, 2018

sighingnow commented May 23, 2018

alpmestan commented May 23, 2018

sighingnow commented May 23, 2018

alpmestan commented May 23, 2018

chitrak7 commented Jun 4, 2018

snowleopard commented Jun 4, 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

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

chitrak7 Jun 12, 2018 • edited Loading

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

snowleopard commented Jun 12, 2018

alpmestan commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Jun 13, 2018

chitrak7 commented May 13, 2018 •

edited

Loading

snowleopard Jun 12, 2018 •

edited

Loading

chitrak7 Jun 12, 2018 •

edited

Loading