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

Revert Smart Assertion Changes #7713

Merged
merged 2 commits into from
Jan 18, 2023
Merged

Revert Smart Assertion Changes #7713

merged 2 commits into from
Jan 18, 2023

Conversation

adamgfraser
Copy link
Contributor

The changes to the smart assertion implementation in #7641 are causing several regressions. In addition to causing null pointer exceptions to be thrown when asserting that a value is equal to null, we no longer have highlighting of which assertion in a composed assertion fails, which was an important feature of the composed assertions.

2.0.5:

image

2.0.6:

image

I think we need to go back and take another crack at this to support rendering assertions while making fewer changes to the execution path for evaluating them.

@regiskuckaertz
Copy link
Member

Can we look at that NPE and missing highlighting? It seems like two easy fixes. But I agree while going through the review that there was some redundancy in the code... and even after that zio-mock could not be updated to work b/c some zio-test layers were marked as package private. I am not quite sure putting the mock in its own library was the right move, it is not working as well as the zio 1 version and the repo has received no attention since last year. We're keen to help get it back on track though, we can take a deeper look at how to make zio-mock and test coexist with few changes.

@adamgfraser adamgfraser merged commit 07a76af into zio:series/2.x Jan 18, 2023
@adamgfraser adamgfraser deleted the assertion branch January 18, 2023 13:05
@adamgfraser
Copy link
Contributor Author

@regiskuckaertz Yes definitely! I think the null pointer exception is an easy fix. The rendering seemed like a more structural issue, at least after I looked at it for a little while. So I thought it made more sense to get back to the previous state and proceed from there. But definitely want to get back to this and if you want to open another version of that PR we can add some more tests and work through the best way to get this working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants