Bug 441 - Avoid unit tests that depend on other unit tests being run first
Summary: Avoid unit tests that depend on other unit tests being run first
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: Other Linux
: --- enhancement
Assignee: Jacob Lifshay
URL:
Depends on:
Blocks: 383
  Show dependency treegraph
 
Reported: 2020-07-26 06:42 BST by Jacob Lifshay
Modified: 2021-04-20 14:45 BST (History)
3 users (show)

See Also:
NLnet milestone: NLNet.2019.10.Wishbone
total budget (EUR) for completion of task and all subtasks: 400
budget (EUR) for this task, excluding subtasks' budget: 400
parent task for budget allocation: 383
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
jacob={amount=200,paid=2020-12-09} "lkcl"={amount=200, paid=2020-08-21}


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Lifshay 2020-07-26 06:42:13 BST
In:
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/test_pipe_caller.py;h=16f0939bc3e4ac18c0b2829035450f7c2995c9e2;hb=8bf37997d31250126a664aeb3bd67ac0cd72a70c#l102

I specifically changed the class creating .test_data to not be built on unittest's infrastructure since the functions that add entries to test_data need to be run before the actual Simulator test case.

unittest (and testing infrastructure built to use it, such as nose3) fundamentally assumes that all unit tests are independent of each other, which allows them to be run in parallel, on different computers, or not even at all.

We should change the code back to not having the code constructing the list of instructions to test be submitted to unittest.

This also should be done for all the other places the subtly incorrect code that I tried to fix was copied to.

Basically, if the code which invokes unittest needs to be something more than just:
if __name__ == "__main__":
    unittest.main()
then you're probably doing something wrong
Comment 1 Luke Kenneth Casson Leighton 2020-07-26 10:23:39 BST
(In reply to Jacob Lifshay from comment #0)
> In:
> https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/
> test_pipe_caller.py;h=16f0939bc3e4ac18c0b2829035450f7c2995c9e2;
> hb=8bf37997d31250126a664aeb3bd67ac0cd72a70c#l102
> 
> I specifically changed the class creating .test_data to not be built on
> unittest's infrastructure since the functions that add entries to test_data
> need to be run before the actual Simulator test case.

i know.  however you didn't add a comment explaining why that had been done.  consequently i looked at it, went, "this will run the test twice, particularly
when run with nosetests3!!" and removed it.

it was only *in passing* on one of the daily updates that i noticed a comment (which should have been in the code) explaining what the evaluation of the dictionary of functions was doing.

so.

a) the names of all of those unit tests (the ones that add entries) need to
be changed to no longer start with the name "test_"

b) we need a base class for all of them (so as not to duplicate the accumulator code)

c) what we are doing needs explanatory code-comments at all times!  this is how we communicate "intent" and not end up at cross-purposes.

d) you identified (hooray) the existence of subTest which, because it was not being used, caused the execution of the list of tests to stop at the first error (which is annoying).  so all test_pipe_caller.py and test_compunit.py need to call subTest.

i'll start on some of that this morning.
Comment 2 Luke Kenneth Casson Leighton 2020-07-26 10:32:18 BST
commit aae58b75a9001b13c9121e8012c4d3fad5ba85ce (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Sun Jul 26 10:32:08 2020 +0100

    use new test accumulator class in div tests

commit 93297fd8dbd7033e13f97f6de6d3674d872a2043
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Sun Jul 26 10:31:49 2020 +0100

    add common test base class for "accumulating" tests to run
Comment 3 Luke Kenneth Casson Leighton 2020-07-26 10:56:23 BST
commit 2e4ea4a16804b987748354e877e917f905286794 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Sun Jul 26 10:56:08 2020 +0100

    get div compunit test running (use new way to accumulate tests)
Comment 4 Luke Kenneth Casson Leighton 2020-07-26 11:43:37 BST
i have 30% of the tests converted.  test_issuer or test_core cannot run until all of them are done.  or they might be able to with some messing about.

i changed the search pattern to "case_" so that nosetest3 will not wildcard pattern match against it and try to execute the function erroneously.
Comment 5 Luke Kenneth Casson Leighton 2020-07-26 12:06:20 BST
ah, oh dear.  the accumulator system needs a means and method of detecting
"skip".  sigh.


    # TODO XXX whoops...
    #@unittest.skip("spr does not have TRAP in it. has to be done another way")
    def _skip_case_3_mtspr_priv(self):
        lst = ["mtspr 26, 1",  # SRR0
               "mtspr 27, 2",  # SRR1
               "mtspr 1, 3",  # XER
               "mtspr 9, 4", ]  # CTR
        initial_regs = [0] * 32
        initial_regs[1] = 0x129518230011feed
        initial_regs[2] = 0x123518230011feed
        initial_regs[3] = 0xe00c0000
        initial_regs[4] = 0x1010101010101010
        initial_sprs = {'SRR0': 0x12345678, 'SRR1': 0x5678, 'LR': 0x1234,
                        'XER': 0x0}
        msr = 1 << MSR.PR
        self.add_case(Program(lst, bigendian),
                             initial_regs, initial_sprs, initial_msr=msr)
Comment 6 Jacob Lifshay 2020-07-26 17:44:35 BST
(In reply to Luke Kenneth Casson Leighton from comment #5)
> ah, oh dear.  the accumulator system needs a means and method of detecting
> "skip".  sigh.

idea: catch and keep the Skip exception in the TestCase so we can rethrow it during the sub-test
Comment 7 Jacob Lifshay 2020-07-29 00:51:11 BST
I cleaned up the div pipe tests to allow running in parallel
Comment 8 Jacob Lifshay 2020-07-29 01:24:13 BST
(In reply to Luke Kenneth Casson Leighton from comment #5)
> ah, oh dear.  the accumulator system needs a means and method of detecting
> "skip".  sigh.

added an initial implementation.
Comment 9 Jacob Lifshay 2020-07-29 01:51:24 BST
working on cleaning up more tests.

Luke, does what I changed so far look good?
Comment 10 Luke Kenneth Casson Leighton 2020-07-29 11:51:03 BST
(In reply to Jacob Lifshay from comment #9)
> working on cleaning up more tests.
> 
> Luke, does what I changed so far look good?

yeah i like it.

btw one thing you may have noticed i did in.. oh! you move it to
DivTestHelper() :)

the levels of indentation particularly when adding subTest() were
far too great.

commit e51ea2436a790173017d2c7766b54c097dd89a9b (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Wed Jul 29 11:49:16 2020 +0100

    move actual ALU test out of subTest indentation just like for div

i've done ALU, DIV, and one other like this, now.  it means less pressure
on indentation and, also interestingly, identifies code that really should
be put into a base class (what's left of run_all / run_it).
Comment 11 Luke Kenneth Casson Leighton 2020-07-29 12:04:09 BST
commit 96845f04045c47f7e1768d7d033b60860f51bce6
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Jul 28 16:49:10 2020 -0700

    clean up div pipe tests to allow them to be run in parallel

niiice :)

oh btw turns out i was overly concerned about pia when
running the div compunit test.  test_div_compunit.py simply doesn't
check against pia because the code in check_cu_outputs is duplicated
in check_alu_outputs.  if the extra argument (pia_res) was added
to the compunit test that *would* break every single one of the
compunit tests (except div).


done these as well:

commit 17e6e88484444bb305375dea1f6a6956a0022987 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Wed Jul 29 11:54:19 2020 +0100

    move SHIFTROT test out of subtest indentation

commit 1ee0d32d99df797f6a11abaf747fec9dce172431 (HEAD -> master, origin/master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Wed Jul 29 11:58:59 2020 +0100

    move CR test out of subtest indentation
Comment 12 Luke Kenneth Casson Leighton 2020-08-18 12:48:46 BST
happy with this, jacob, EUR 200 / 200