Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration tests #85

Merged
merged 13 commits into from
Jan 17, 2024
Merged

Integration tests #85

merged 13 commits into from
Jan 17, 2024

Conversation

funkyfuture
Copy link
Contributor

This adds an initial framework for tests against larger sets of real-world corpora.

A mixed bag of fixes for discovered bugs comes with it.

Let me know if we should look at it together in person.

This PR has higher priority than #82. I still need to find the opportunity to create new graphics for that.

I'll continue to work on serialization based on the additions here.

This an extra effort to deal with the lxml-backend, but it's more
important to define a consistent behaviour.

It's noteworthy that the keys of the attributes could also consist of
two-value tuples with namespace and local name.
This is a revert of a2c7d55 that would
lead to crashes when documents have a funky mixture of several default
namespaces.
Due to that removal the hashing of Namespace objects isn't needed
anymore and get removed as well.
Where the funk is, the element wrapper cache comes around to bang.
Bless the funk when the effing cache is gone.
as lxml's implementation doesn't represent a default namespace in
etree._Attrib (that's the backend data structure of TagAttributes).
Explicitly declared namespace weren't considered before.
@funkyfuture funkyfuture added bug Something isn't working enhancement New feature or request labels Jan 15, 2024
@funkyfuture funkyfuture added this to the 0.5 milestone Jan 15, 2024
@JKatzwinkel
Copy link
Contributor

looks awesome! i'd need some time to set it up and check it out but i dont see why this shouldnt be merged. lets just set required coverage down to 96% to make the ci stop complaining!

@JKatzwinkel
Copy link
Contributor

as an aside, i very much enjoy modeling a dsl for integration test scenarios (as you already did) and then extract scenario specs out of the test suite code and into yaml files early on. really adds to developing speed and overall perceived comfort in my experience. maybe keep something like this in mind for consideration sometime later?

JKatzwinkel
JKatzwinkel previously approved these changes Jan 17, 2024
note: python3.7 is the only environment in which 97% is not reached currently.
@funkyfuture
Copy link
Contributor Author

looks awesome! i'd need some time to set it up and check it out but i dont see why this shouldnt be merged. lets just set required coverage down to 96% to make the ci stop complaining!

since it only affects Python 3.7:

  • i'd have rather just disabled the coverage constraint for this version / or enable it only for the latest interpreter version
  • my hope is still, that it would hit the expectation when the whole serialization code is there

it's okay for now though.

@funkyfuture
Copy link
Contributor Author

as an aside, i very much enjoy modeling a dsl for integration test scenarios (as you already did) and then extract scenario specs out of the test suite code and into yaml files early on. really adds to developing speed and overall perceived comfort in my experience. maybe keep something like this in mind for consideration sometime later?

i can't really follow. feel free to open an issue with a practice oriented sketch.

the next addition test-wise will be the comparison of a pretty serialisat in conjunction with collapsed whitespace.

@JKatzwinkel
Copy link
Contributor

looks awesome! i'd need some time to set it up and check it out but i dont see why this shouldnt be merged. lets just set required coverage down to 96% to make the ci stop complaining!

since it only affects Python 3.7:

  • i'd have rather just disabled the coverage constraint for this version / or enable it only for the latest interpreter version
  • my hope is still, that it would hit the expectation when the whole serialization code is there

it's okay for now though.

personally, I can make my peace with a 96% coverage minimum, but don't let me stop you applying the gate selectively for all versions except 3.7 because I have no idea how one would do that.

@funkyfuture
Copy link
Contributor Author

personally, I can make my peace with a 96% coverage minimum

well, it's a regression. but getting rid of the lxml backend will result in something very different code-volume-wise and 99% will be the target then.

and these tests increase edge case coverage a lot, that's way more important than a stupid number.

because I have no idea how one would do that.

something with cli arguments i guess. but it's simple enough now.

@funkyfuture funkyfuture merged commit d5359ef into main Jan 17, 2024
9 checks passed
@funkyfuture
Copy link
Contributor Author

thanks for the feedback. i hope the other branches rebased themselves when i arrive home.

@funkyfuture funkyfuture deleted the integration-tests branch January 17, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants