Move expected tracer.events from test code to golden files #133

victorjzsun · 2021-08-26T02:38:45Z

Addresses #125. Replaces certain assertions to instead be checked using golden files.

laike9m · 2021-08-27T07:31:55Z

Thanks for taking care of this. Actually what I meant in #125 is that, we still keep the assertions for tracer.events, but instead of writing something like assert trace.events == [...] in test code, we modify this check to something similar to the check of mocked_responses, aka using json.

You removed the assert trace.events == [...], but I think they still provide values by ensuring the correctness of an eariler stage. This can help ease dedebugging, because we know at which stage (generation of events, or generation of RPC payload) the data goes wrong.

victorjzsun · 2021-08-27T21:32:45Z

Could we generate the trace.events from the existing json data files? the events should be the same would seem a bit redundant to be put seperately. It would mean diff'ing the actual to intended might be a bit harder

laike9m · 2021-08-28T19:15:34Z

Generating trace.events JSON from the existing json data files is fine, but eventually we need to be able to dump these JSON from the trace.events in memory. Imagine we add a field to the events, and we definitely don't want to manually update the golden files. We already support the golden file override feature for mock response, and it would be the same for trace.events.

laike9m · 2021-10-04T05:44:25Z

I'm working on #126 to support Python 3.10. 3.10 changes bytecode quite a bit, so hard-code trace.events in test doesn't work anymore. I'm looking forward to this feature😀

Btw, do you think get_value in test/utils.py can be removed once this is in?

victorjzsun · 2021-10-04T22:53:22Z

I'm working on #126 to support Python 3.10. 3.10 changes bytecode quite a bit, so hard-code trace.events in test doesn't work anymore. I'm looking forward to this feature😀

Btw, do you think get_value in test/utils.py can be removed once this is in?

get_value is also used in tracer.loops assertions so not yet. I was planning on just focusing on tracer.events for this PR and deal with tracer.loops assertions in the next PR.

laike9m · 2021-10-05T06:56:53Z

cyberbrain/basis.py

@@ -64,6 +64,8 @@ class Event:

    @staticmethod
    def value_serializer(inst: type, field: attr.Attribute, value: Any):
+        if field is None:


why do we need this?

It's not particularly needed for this PR but if you use it in attr.asdict without the filter removing sources, it throws an error. This is due to it recursing into the sources list and becoming simple types with no fields. I added this as a sanity check to avoid that error if we happen to use attr.asdict for other purposes. If you don't think it's necessary, I'll remove it.

We can leave it here and add a comment

laike9m · 2021-10-05T07:09:03Z

test/conftest.py

@@ -73,4 +126,16 @@ def mocked_responses(request):
        if get_os_type() == "windows" and frame_name in {"test_numpy", "test_pandas"}:
            return

-        assert frame_data == golden_frame_data, json.dumps(frame_data, indent=4)
+        if golden_frame_data.get("response", None) is None:


If default is not given, it defaults to None

so None is not needed here

laike9m · 2021-10-05T07:24:04Z

test/conftest.py

 @pytest.fixture
-def mocked_responses(request):
+def check_response(request):


I feel like this function is doing multiple things and we should break into smaller ones, something like

golden_response = get_golden_response() if golden_response: assert ... else: update_golden_response()

This makese the intent more clear. A good side effect: skip assertion if golden_response does not exist (now assertion still happens if the file exists).

Same with check_tracer_events

laike9m · 2021-10-05T07:30:32Z

test/conftest.py

+        snapshot = {
+            "location": symbol.snapshot.location,
+            "events_pointer": symbol.snapshot.events_pointer,
+        }


you can do

# If snapshot is None, keep it. snapshot = snapshot and { "location": symbol.snapshot.location, "events_pointer": symbol.snapshot.events_pointer, }

laike9m · 2021-10-05T07:31:18Z

test/conftest.py

+
+    yield
+
+    tracer_events = []


this can be extracted to a separate function.

laike9m

left some comments

laike9m · 2021-10-06T18:09:19Z

will do

…

On Wed, Oct 6, 2021 at 9:21 AM Victor Sun ***@***.***> wrote: @victorjzsun <https://github.com/victorjzsun> requested your review on: #133 <#133> Move assertions to golden files. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#133 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATY3TK27LLHPETDYYUQQATUFRZSHANCNFSM5C2LTEHQ> .

laike9m · 2021-10-07T04:27:12Z

test/conftest.py

+def get_golden_data(golden_filepath, key):
+    directory = os.path.dirname(golden_filepath)


laike9m · 2021-10-07T04:27:31Z

test/conftest.py

+def update_golden_data(golden_filepath, key, value):
+    golden_frame_data = {}


laike9m · 2021-10-07T04:32:13Z

test/conftest.py

+def get_serialized_events():
+    tracer_events = []


Please add a doc string with a simple example.

laike9m · 2021-10-07T04:35:05Z

Overall it looks good now. Just a few nits.

laike9m · 2021-10-07T04:55:22Z

Btw, once you're done with the modification, could you squash the commits? I've configured this repo to support it, but I'm not sure if you'll see a button or something else. Let me know if you met any issues.

Nvm, I just find that I can squash myself

victorjzsun · 2021-10-07T14:18:40Z

Nvm, I just find that I can squash myself

Only collaborators have merge access so I can't even merge it regardless.

laike9m · 2021-10-07T21:22:15Z

Perhaps I should add you as a collaborator then🙂 let me think about it

laike9m · 2021-10-07T21:25:26Z

Anything else you want to add? If not, Im going to merge it.

victorjzsun · 2021-10-07T22:01:23Z

Nope, go ahead 🙂

laike9m · 2021-10-08T03:42:17Z

Merged. Thanks again for the great contribution!

Move assertions to golden files

24a7716

victorjzsun added 9 commits September 15, 2021 22:51

Implement new fixture for asserting tracer.events

8a55728

Add mocked_responses to all tests

ad6d847

Add class name to event

d099926

Remove import test due to repr naming

6f05635

Formatting

a9a4b02

Use both fixtures

575227c

Remove unnecessary fixture

3d823f8

Remove tracer.events assertions

c38c778

Fix test

fdc4f7a

Remove extra assert

46dbcb4

victorjzsun marked this pull request as ready for review October 4, 2021 23:59

Renaming

8effcbf

laike9m reviewed Oct 5, 2021

View reviewed changes

test/conftest.py

yield

tracer_events = []

Copy link

Owner

laike9m Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be extracted to a separate function.

laike9m requested changes Oct 5, 2021

View reviewed changes

victorjzsun added 2 commits October 5, 2021 23:38

Reformatting as per comments

be40db8

Lint

d7c0d1e

victorjzsun requested a review from laike9m October 6, 2021 16:21

laike9m reviewed Oct 7, 2021

View reviewed changes

test/conftest.py Outdated

Comment on lines 57 to 58

def update_golden_data(golden_filepath, key, value):

golden_frame_data = {}

Copy link

Owner

laike9m Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

laike9m reviewed Oct 7, 2021

View reviewed changes

Add docs

0e75b80

laike9m changed the title ~~Move assertions to golden files~~ Move expected tracer.events from test code to golden files Oct 8, 2021

laike9m merged commit 30957ad into laike9m:master Oct 8, 2021

victorjzsun deleted the consolidate-tracer-asserts branch October 8, 2021 03:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move expected tracer.events from test code to golden files #133

Move expected tracer.events from test code to golden files #133

victorjzsun commented Aug 26, 2021

laike9m commented Aug 27, 2021 •

edited

Loading

victorjzsun commented Aug 27, 2021

laike9m commented Aug 28, 2021

laike9m commented Oct 4, 2021 •

edited

Loading

victorjzsun commented Oct 4, 2021

laike9m Oct 5, 2021

victorjzsun Oct 6, 2021 •

edited

Loading

laike9m Oct 6, 2021

laike9m Oct 5, 2021 •

edited

Loading

laike9m Oct 5, 2021

laike9m Oct 5, 2021

laike9m Oct 5, 2021 •

edited

Loading

laike9m Oct 5, 2021

laike9m left a comment

laike9m commented Oct 6, 2021 via email

laike9m Oct 7, 2021

laike9m Oct 7, 2021

laike9m Oct 7, 2021

laike9m commented Oct 7, 2021

laike9m commented Oct 7, 2021 •

edited

Loading

victorjzsun commented Oct 7, 2021

laike9m commented Oct 7, 2021

laike9m commented Oct 7, 2021 •

edited

Loading

victorjzsun commented Oct 7, 2021 •

edited

Loading

laike9m commented Oct 8, 2021

		def get_golden_data(golden_filepath, key):
		directory = os.path.dirname(golden_filepath)

		def update_golden_data(golden_filepath, key, value):
		golden_frame_data = {}

Move expected tracer.events from test code to golden files #133

Move expected tracer.events from test code to golden files #133

Conversation

victorjzsun commented Aug 26, 2021

laike9m commented Aug 27, 2021 • edited Loading

victorjzsun commented Aug 27, 2021

laike9m commented Aug 28, 2021

laike9m commented Oct 4, 2021 • edited Loading

victorjzsun commented Oct 4, 2021

Choose a reason for hiding this comment

victorjzsun Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laike9m Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laike9m Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laike9m left a comment

Choose a reason for hiding this comment

laike9m commented Oct 6, 2021 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laike9m commented Oct 7, 2021

laike9m commented Oct 7, 2021 • edited Loading

victorjzsun commented Oct 7, 2021

laike9m commented Oct 7, 2021

laike9m commented Oct 7, 2021 • edited Loading

victorjzsun commented Oct 7, 2021 • edited Loading

laike9m commented Oct 8, 2021

laike9m commented Aug 27, 2021 •

edited

Loading

laike9m commented Oct 4, 2021 •

edited

Loading

victorjzsun Oct 6, 2021 •

edited

Loading

laike9m Oct 5, 2021 •

edited

Loading

laike9m Oct 5, 2021 •

edited

Loading

laike9m commented Oct 7, 2021 •

edited

Loading

laike9m commented Oct 7, 2021 •

edited

Loading

victorjzsun commented Oct 7, 2021 •

edited

Loading