Tester Refactor #22

rsetaluri · 2018-08-16T02:43:30Z

Started refactoring the tester and targets. Wanted to get some feedback on this -- still more to do if the direction is good. The commit msg should describe the new way of doing things.

@leonardt what do you think?

rsetaluri · 2018-08-16T02:44:19Z

tests/test_tester.py

@@ -10,9 +10,9 @@ def test_tester_basic():
    tester = fault.Tester(circ)


the serialization tests should be disjoint from the tester tests. the tester tests should just ensure that the series of actions generated is accurate (mostly trivial).

rsetaluri · 2018-08-16T02:45:12Z

tests/test_value_utils.py

+          "d", m.In(m.Array(16, m.Array(20, m.Bit)))]
+
+
+def test_all():


This test could be structured better, but just wanted to get in all the test functionality so we're sure nothing is broken.

pytest parametrization might be useful here (see https://docs.pytest.org/en/latest/parametrize.html)

rsetaluri · 2018-08-16T02:45:34Z

fault/verilator_target.py

+                    f"{i}, \"{action.port.name}\");"]
+        if isinstance(action, actions.Eval):
+            return ["top->eval();"]
+        if isinstance(action, actions.Step):


rsetaluri · 2018-08-16T02:46:23Z

fault/tester.py

+                self.vectors[-1][index] = make_value(port, AnyValue)
+
+    def process(self, action):
+        if isinstance(action, (actions.Poke, actions.Expect)):


need to deal with nested arrays here (i.e. tester.poke(circ.I[2]))

Yes, I think this logic is what you're referring to https://github.com/leonardt/fault/pull/22/files#diff-cbd9d87f93cddae35c5f407fde349e05L63

rsetaluri · 2018-08-16T02:46:51Z

fault/verilator_target.py

+
+    @staticmethod
+    def generate_action_code(i, action):
+        if isinstance(action, actions.Poke):


need to deal with nested arrays --> flatten names

Yes, the array flattening stuff is quite annoying, I'm not sure if there's a less complex solution to this.

leonardt

Changes look good to me, carry on!

leonardt · 2018-08-16T21:33:34Z

fault/actions.py

+    def __init__(self, steps, clock):
+        super().__init__()
+        self.steps = steps
+        self.clock = clock


Looks good to me, I like this abstraction of Actions. I thought about this more, and I think this is the right approach in general. It's much more conservative in space (it records the changes to the input/output state of the circuit, rather than the entire input/output state contained in a full test vector). This state is realized in the simulation state itself, so there's no reason to duplicate the state in the test vector.

We can always generate the test vector version from this sequence of actions.

leonardt · 2018-08-16T21:43:11Z

fault/tester.py

+                self.vectors[-1][index] = make_value(port, AnyValue)
+
+    def process(self, action):
+        if isinstance(action, (actions.Poke, actions.Expect)):


Yes, I think this logic is what you're referring to https://github.com/leonardt/fault/pull/22/files#diff-cbd9d87f93cddae35c5f407fde349e05L63

leonardt · 2018-08-16T22:15:23Z

fault/verilator_target.py

+
+    @staticmethod
+    def generate_action_code(i, action):
+        if isinstance(action, actions.Poke):


Yes, the array flattening stuff is quite annoying, I'm not sure if there's a less complex solution to this.

leonardt · 2018-08-16T22:16:33Z

tests/test_value_utils.py

+          "d", m.In(m.Array(16, m.Array(20, m.Bit)))]
+
+
+def test_all():


pytest parametrization might be useful here (see https://docs.pytest.org/en/latest/parametrize.html)

rsetaluri · 2018-08-17T17:39:57Z

@leonardt this should be ready for final review.

Refactoring tester to now just capture a sequence of "actions". Each target can choose to interpret these actions however. These actions tend to be more 1-to-1 with actions in the actual targets, making the translation easier. The verilator target has been mostly fully implemented. TODO: -- Verilator target nested arrays + name flattening -- Python/CoreIR simulator target

tests/test_tester.py only tests how Tester produces actions (not serialization). The serialization is tested specifically by testing TestVectorBuilder (in tests/test_test_vector_builder.py).

Added the following: -- Logic to optinally skip compilation to verilog in the target -- Nested array support (flattening). This includes a utility function "verilator_name()" in verilator_utils.py.

Updated the magma simulator targets to work with the new action interface. Note that the python simulator is not supported and not tested for now. We issue a warning if it is used.

Renamed the corresponding class and files. This is to avoid pytest warnings etc. Also makes the names cleaner.

Rather than compiling to verilator we now just check that the test vectors have the structure we expect. This isolates the behavior the test vector generation.

coveralls · 2018-08-19T16:49:44Z

Pull Request Test Coverage Report for Build 166

239 of 270 (88.52%) changed or added relevant lines in 9 files are covered.
10 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.8%) to 79.074%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
fault/verilator_utils.py	16	17	94.12%
fault/vector_builder.py	50	52	96.15%
fault/test_vectors.py	7	9	77.78%
fault/actions.py	23	25	92.0%
fault/verilator_target.py	56	59	94.92%
fault/value_utils.py	36	40	90.0%
fault/magma_simulator_target.py	24	29	82.76%
fault/tester.py	24	36	66.67%

Files with Coverage Reduction	New Missed Lines	%
fault/magma_simulator_target.py	1	82.35%
fault/array.py	4	59.09%
fault/test_vectors.py	5	87.8%

Totals
Change from base Build 149:	-0.8%
Covered Lines:	427
Relevant Lines:	540

💛 - Coveralls

leonardt · 2018-08-20T15:47:07Z

@rsetaluri looks like the most recent checks passed. Is this ready for a final review?

rsetaluri · 2018-08-20T15:52:09Z

@leonardt yes it is ready for final review

leonardt

Looks good, minor change request but I'll make it and do the merge. Thanks for the great work. The verilator target code looks much cleaner, and I think working with Actions is going to lead to better internal code (rather than dealing with test vector indices and such)

leonardt · 2018-08-27T22:49:25Z

tests/test_magma_simulator_target.py

    circ = common.TestNestedArraysCircuit
-    tester = fault.Tester(circ)
-    expected = [random.randint(0, (1 << 4) - 1) for i in range(3)]
+    expected = [_BV(random.randint(0, (1 << 4) - 1)) for i in range(3)]


This could use random.rand_bv

rsetaluri commented Aug 16, 2018

View reviewed changes

leonardt reviewed Aug 16, 2018

View reviewed changes

rsetaluri force-pushed the refactor branch from dd9a771 to bd2b162 Compare August 17, 2018 17:41

rsetaluri added 17 commits August 19, 2018 08:53

Add value utils and test

05cca4d

Check that clock is a clock type

19b2b26

Add support for nested arrays in vector builder

2954fce

Move test_vector_builder into separate file

2dfb5c7

Fix wrong error message

5724d9e

Refactor tester tests

1c30940

tests/test_tester.py only tests how Tester produces actions (not serialization). The serialization is tested specifically by testing TestVectorBuilder (in tests/test_test_vector_builder.py).

Update verilator target

8fd110c

Added the following: -- Logic to optinally skip compilation to verilog in the target -- Nested array support (flattening). This includes a utility function "verilator_name()" in verilator_utils.py.

Update verilator target tests

5b4bfe9

Add Step() support in verilator target

d8dac33

Add clock test for verilator target

4eb3087

Update magma simulator targets

ae31d53

Updated the magma simulator targets to work with the new action interface. Note that the python simulator is not supported and not tested for now. We issue a warning if it is used.

Issue warning for python target

c164113

Rename 'test vector builder' to 'vector builder'

1168d6b

Renamed the corresponding class and files. This is to avoid pytest warnings etc. Also makes the names cleaner.

Allow BitVector inputs to make_value()

9562d8b

Fix formatting error

7cdf175

Fix test vector tests

de62b5d

Rather than compiling to verilator we now just check that the test vectors have the structure we expect. This isolates the behavior the test vector generation.

rsetaluri force-pushed the refactor branch from 9e1d0d3 to de62b5d Compare August 19, 2018 15:56

rsetaluri added 5 commits August 19, 2018 09:09

Switch from lambda to def

80a72b1

Skip verilog compilation in test

c9c0213

Don't emit code for expect(Any) for verilator

7b531c0

Reformat generated verilator code

f9a275d

Fix misspelling

ce4be91

leonardt approved these changes Aug 27, 2018

View reviewed changes

leonardt added 3 commits August 27, 2018 15:54

Use random_bv for simulator test

663ae7e

Fix issue with simulator test vectors

3d36c71

Fix pep8

56b526d

leonardt merged commit 26e09fa into master Aug 27, 2018

leonardt deleted the refactor branch August 27, 2018 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tester Refactor #22

Tester Refactor #22

rsetaluri commented Aug 16, 2018 •

edited

Loading

rsetaluri Aug 16, 2018

rsetaluri Aug 17, 2018 •

edited

Loading

rsetaluri Aug 16, 2018

leonardt Aug 16, 2018

rsetaluri Aug 16, 2018

rsetaluri Aug 17, 2018

rsetaluri Aug 16, 2018

leonardt Aug 16, 2018

rsetaluri Aug 17, 2018

rsetaluri Aug 16, 2018

leonardt Aug 16, 2018

rsetaluri Aug 17, 2018

leonardt left a comment

leonardt Aug 16, 2018

leonardt Aug 16, 2018

leonardt Aug 16, 2018

leonardt Aug 16, 2018

rsetaluri commented Aug 17, 2018

coveralls commented Aug 19, 2018 •

edited

Loading

leonardt commented Aug 20, 2018

rsetaluri commented Aug 20, 2018

leonardt left a comment

leonardt Aug 27, 2018

		@@ -10,9 +10,9 @@ def test_tester_basic():
		tester = fault.Tester(circ)

Tester Refactor #22

Tester Refactor #22

Conversation

rsetaluri commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

rsetaluri Aug 17, 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

leonardt 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

rsetaluri commented Aug 17, 2018

coveralls commented Aug 19, 2018 • edited Loading

Pull Request Test Coverage Report for Build 166

💛 - Coveralls

leonardt commented Aug 20, 2018

rsetaluri commented Aug 20, 2018

leonardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsetaluri commented Aug 16, 2018 •

edited

Loading

rsetaluri Aug 17, 2018 •

edited

Loading

coveralls commented Aug 19, 2018 •

edited

Loading