search

laike9m / Cyberbrain

Python debugging, redefined.
http://bit.ly/cyberbrain-features
MIT License
2.51k stars 159 forks source link

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

Closed victorjzsun closed 2 years ago

victorjzsun commented 3 years ago

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

laike9m commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

will do

On Wed, Oct 6, 2021 at 9:21 AM Victor Sun @.***> wrote:

@victorjzsun https://github.com/victorjzsun requested your review on:

133 https://github.com/laike9m/Cyberbrain/pull/133 Move assertions to

golden files.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/laike9m/Cyberbrain/pull/133#event-5421770933, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATY3TK27LLHPETDYYUQQATUFRZSHANCNFSM5C2LTEHQ .

laike9m commented 2 years ago

Overall it looks good now. Just a few nits.

laike9m commented 2 years ago

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 image

victorjzsun commented 2 years ago

Nvm, I just find that I can squash myself

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

laike9m commented 2 years ago

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

laike9m commented 2 years ago

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

victorjzsun commented 2 years ago

Nope, go ahead 🙂

laike9m commented 2 years ago

Merged. Thanks again for the great contribution!