Serialization hooks by nicoddemus · Pull Request #4965 · pytest-dev/pytest

nicoddemus · 2019-03-20T23:52:50Z

Add pytest_report_serialize and pytest_report_unserialize experimental hooks.

Our intention is to keep this code closer to the core, given that it
might break easily due to refactorings.

Having it in the core might also allow to improve the code by moving
some responsibility to the "code" objects (ReprEntry, etc) which
are often found in the reports.

The actual job is done by _to_json and _from_json of TestReport and CollectReport (this methods were moved from xdist (ca03269)).

These hooks will be used by pytest-xdist, pytest-subtests, and the replacement for resultlog to serialize and customize reports.

nicoddemus · 2019-03-21T00:00:48Z

I noticed that we have arbitrary keyword arguments that can be passed to TestReport and CollectReport; those were added a long time ago in 5263656 by @RonnyPfannschmidt. I wonder if now we can get rid of them and use custom reports in xdist with the new hooks? Then we could also convert TestReport and CollectReport to attrs.

Found another use case here: https://github.com/pytest-dev/pytest/pull/1874/files.

RonnyPfannschmidt · 2019-03-21T10:53:53Z

@nicoddemus as long as reports support random mutation/extension, its necessary to support unknown attributes in some way,

so we would need an "extension point" style attribute as a starting point to create a migration path

details like the optional worker id are currently just "tacked on"

nicoddemus · 2019-03-21T10:58:35Z

so we would need an "extension point" style attribute as a starting point to create a migration path

Yes, I was thinking of having an explicit extra_args={} or something like that instead of **kwargs. One reason I would like to ditch kwargs is that apparently attrs doesn't support it.

RonnyPfannschmidt · 2019-03-21T10:59:58Z

the critical point here is api consumers and backward compatibility

nicoddemus · 2019-03-21T11:06:42Z

Definitely, we can probably introduce a deprecation for **kwargs and direct users to use a new extras dict instead. What do you think?

RonnyPfannschmidt · 2019-03-21T11:17:16Z

i dont see report construction as the issue - but rather report consumption

nicoddemus · 2019-03-21T11:20:09Z

i dont see report construction as the issue - but rather report consumption

So it would probably be OK to change the report classes to attrs, while changing the __init__ signature. attrs class are normal classes, so assigning attributes to the instances as is done now would continue to work.

RonnyPfannschmidt · 2019-03-21T11:37:38Z

pytest does not expose the report type so its absolutely valid to change its signature, however it does expose report objects, so its necessary to keep the object signature

a combination of slots + redirecting setattr could do the trick,, there will be some "pain"

nicoddemus · 2019-03-21T17:35:59Z

Thanks for the thoughts @RonnyPfannschmidt. 👍

Regardless this is certainly a topic for a separate PR.

codecov · 2019-03-21T22:00:41Z

Codecov Report

Merging #4965 into features will increase coverage by 0.02%.
The diff coverage is 98.52%.

@@             Coverage Diff             @@
##           features   #4965      +/-   ##
===========================================
+ Coverage     95.97%     96%   +0.02%     
===========================================
  Files           113     114       +1     
  Lines         25359   25631     +272     
  Branches       2501    2536      +35     
===========================================
+ Hits          24339   24607     +268     
  Misses          714     714              
- Partials        306     310       +4

Impacted Files	Coverage Δ
src/_pytest/config/__init__.py	`94.4% <ø> (ø)`	⬆️
testing/test_reports.py	`100% <100%> (ø)`
src/_pytest/hookspec.py	`100% <100%> (ø)`	⬆️
src/_pytest/reports.py	`95% <95.5%> (+0.4%)`	⬆️
testing/test_warnings.py	`98.9% <0%> (ø)`	⬆️
testing/test_session.py	`100% <0%> (ø)`	⬆️
testing/acceptance_test.py	`98.03% <0%> (ø)`	⬆️
testing/test_terminal.py	`99.81% <0%> (ø)`	⬆️
testing/test_capture.py	`99.24% <0%> (ø)`	⬆️
src/_pytest/warnings.py	`88.88% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6eff306...65c8e8a. Read the comment docs.

This methods were moved from xdist (ca03269). Our intention is to keep this code closer to the core, given that it might break easily due to refactorings. Having it in the core might also allow to improve the code by moving some responsibility to the "code" objects (ReprEntry, etc) which are often found in the reports. Finally pytest-xdist and pytest-subtests can use those functions instead of coding it themselves.

These hooks will be used by pytest-xdist and pytest-subtests to serialize and customize reports.

nicoddemus · 2019-03-26T12:08:13Z

I think the coverage looks better now, what do you say @blueyed?

@asottile, do you think you might have a chance to review this sometime?

blueyed

👍

asottile

one comment, one thing to fix -- otherwise looks good

asottile · 2019-03-26T15:24:24Z

    they fail).
    """

+    __test__ = False


heh neat, testify used the same approach: https://github.com/Yelp/Testify/blob/335f04a7de583026a1e0d04d006f4c40524fd89d/testify/test_discovery.py#L57-L59

I think this oddity could really be purged if tests were required to be defined in the module they appear in, and then importing names wouldn't add them to discovery -- always was a weird thing about unittest in my mind -- fwiw I ensured that in testify with this code

takluyver · 2019-03-27T14:33:46Z

+    if isinstance(report, (TestReport, CollectReport)):
+        data = report._to_json()
+        data["_report_type"] = report.__class__.__name__
+        return data


Nitpick: these implementations don't actually serialize/unserialize the objects: they convert them to/from native Python types which can be serialised/deserialised as JSON. Serialisation, to my mind, means converting something to bytes (or at least str).

That's a fine API if that's what you want, but maybe the names should reflect it. Sorry for bikeshedding. ;-)

Not at all, thanks for chipping in. I agree with you, they are not actually serializing anything.

I'm fine with changing those names now, easier to do this before merging. Do you have a suggestion?

Maybe pytest_report_to_json and pytest_report_from_json? It's not strictly accurate either, because they're producing dicts suitable for JSON, not actual JSON, but I think that inaccuracy is widely accepted.

Otherwise:

pytest_report_to_dict - accurate but less specific

pytest_report_to_jsonable - kind of ugly, IMO

I would like to avoid restricting this to "json", because we might change the actual representation to something else.

I like pytest_report_to_dict and pytest_report_from_dict; I agree it is less accurate, but I believe it is enough to mention that it only supports built-in types in the docs.

Thanks a lot for the input! I will change to pytest_report_to_dict and pytest_report_from_dict later then. 👍

at $previous_job we called this def __primitive__(self):

JSON-compatible is a stricter standard than built-in types (e.g. no sets in JSON), but so long as the requirements are documented, I think the dict naming is a good option.

What about …_to_python and …_from_python? But it does not clearly indicate the overall direction (as with …serialize and …unserialize).

to/from_serializable just to keep the door open for msgpack and yaml as well

I like @RonnyPfannschmidt's, we describe the intent rather what the hook returns in detail. 👍

Done, pytest_report_from_serializable/pytest_report_to_serializable. 👍

If nobody objects I will merge this later then.

nicoddemus force-pushed the serialization-hooks branch from acabb78 to 8b683f6 Compare March 20, 2019 23:54

nicoddemus changed the title ~~Serialization hooks~~ WIP Serialization hooks Mar 21, 2019

nicoddemus requested review from asottile and blueyed March 21, 2019 17:35

blueyed reviewed Mar 21, 2019

View reviewed changes

Comment thread src/_pytest/config/__init__.py

Comment thread src/_pytest/hookspec.py Outdated

Comment thread src/_pytest/hookspec.py Outdated

Comment thread src/_pytest/reports.py Outdated

nicoddemus force-pushed the serialization-hooks branch from 2dc6aeb to 3d63d92 Compare March 21, 2019 21:39

pytest-dev deleted a comment from codecov Bot Mar 21, 2019

nicoddemus changed the title ~~WIP Serialization hooks~~ Serialization hooks Mar 21, 2019

nicoddemus mentioned this pull request Mar 21, 2019

Provide an alternative to --result-log #4488

Closed

blueyed mentioned this pull request Mar 22, 2019

config: add essential_plugins list, split out of default_plugins #4976

Closed

nicoddemus added 8 commits March 25, 2019 20:16

Add pytest_report_serialize and pytest_report_unserialize hooks

7b9a414

These hooks will be used by pytest-xdist and pytest-subtests to serialize and customize reports.

Make sure TestReports are not collected as test classes

d856f4e

Code review suggestions

f2e0c74

Add CHANGELOG

6457742

Note that tests from xdist reference the correct xdist issues

e4eec34

Improve coverage for to_json() with paths in reports

ceef0af

Improve coverage for _report_unserialization_failure

2d77018

nicoddemus force-pushed the serialization-hooks branch from 73b70de to 2d77018 Compare March 25, 2019 23:17

blueyed approved these changes Mar 26, 2019

View reviewed changes

asottile approved these changes Mar 26, 2019

View reviewed changes

Fix assertion in pytest_report_unserialize

9311d82

takluyver reviewed Mar 27, 2019

View reviewed changes

Rename hooks: to/from_serializable

65c8e8a

nicoddemus merged commit a9fe1e1 into pytest-dev:features Mar 28, 2019

nicoddemus deleted the serialization-hooks branch March 28, 2019 23:22

nicoddemus mentioned this pull request Apr 2, 2019

add a hook for serializing/unserializing report sections #111

Closed

drothlis mentioned this pull request May 16, 2019

Can't use -p no:python since 4.5.0 #5277

Closed

Uh oh!

Uh oh!

Conversation

nicoddemus commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonnyPfannschmidt commented Mar 21, 2019

Uh oh!

nicoddemus commented Mar 21, 2019

Uh oh!

RonnyPfannschmidt commented Mar 21, 2019

Uh oh!

nicoddemus commented Mar 21, 2019

Uh oh!

RonnyPfannschmidt commented Mar 21, 2019

Uh oh!

nicoddemus commented Mar 21, 2019

Uh oh!

RonnyPfannschmidt commented Mar 21, 2019

Uh oh!

nicoddemus commented Mar 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nicoddemus commented Mar 26, 2019

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicoddemus commented Mar 20, 2019 •

edited

Loading

nicoddemus commented Mar 21, 2019 •

edited

Loading

codecov Bot commented Mar 21, 2019 •

edited

Loading

nicoddemus Mar 27, 2019 •

edited

Loading