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

Cut down on noise in expected strings #54

Closed
ktbarrett opened this issue Jul 31, 2021 · 19 comments
Closed

Cut down on noise in expected strings #54

ktbarrett opened this issue Jul 31, 2021 · 19 comments

Comments

@ktbarrett
Copy link

Many times the full expectation string is long and includes low value information. I'd like to introduce some way to cut down on that noise. One thought is to make the expected string a fullmatch regex. This could also handle #45.

- case: test_logic_promotion
  main: |
    from hdltypes.logic import StdLogic, X01Z, Bit

    a: StdLogic
    b: X01Z
    c: Bit

    reveal_type(c & a) # N: .* "hdltypes.logic.StdLogic(?:\\*|`-?\\d+)"
    reveal_type(c & b) # N: .* "hdltypes.logic.X01Z(?:\\*|`-?\\d+)"
    reveal_type(c & c) # N: .* "hdltypes.logic.Bit(?:\\*|`-?\\d+)"

It might also be possible to make the expected strings templates to enable reuse, and perhaps package some of the common patterns with the tool.

- case: test_logic_promotion
  main: |
    # ...

    reveal_type(c & a) # N: {reveal} "hdltypes.logic.StdLogic{_}"
    reveal_type(c & b) # N: {reveal} "hdltypes.logic.X01Z{_}"
    reveal_type(c & c) # N: {reveal} "hdltypes.logic.Bit{_}"
  meta:
    reveal: "Revealed type is"
    _: "(?:\\*|`-?\\d+)"

The language here is strings, so using the common tools for that: regexes and templates, makes a lot of sense.

@sobolevn
Copy link
Member

Related #45

@brunns
Copy link
Contributor

brunns commented Aug 6, 2021

I'd be happy to work on a PR for this if it would be welcome.

@sobolevn
Copy link
Member

sobolevn commented Aug 6, 2021

@brunns it is! Thank you!

@brunns
Copy link
Contributor

brunns commented Aug 6, 2021

Is there a one-liner for running the tests, or do I create and activate a venv by hand, then run pytest?

@sobolevn
Copy link
Member

sobolevn commented Aug 6, 2021

Yes, activate your venv then https://github.com/typeddjango/pytest-mypy-plugins/blob/master/.github/workflows/test.yml#L24-L29

@brunns
Copy link
Contributor

brunns commented Aug 6, 2021

I'm assuming we'd want a flag to activate this behavior. Otherwise,, the change wouldn't be backward compatible.

@sobolevn
Copy link
Member

sobolevn commented Aug 6, 2021

@brunns yes 👍

@brunns
Copy link
Contributor

brunns commented Aug 6, 2021

So, two options really. There could be have a flag level test, or the comment could be annotated somehow, so we could mix the two styles. Or, I suppose, both? I've made a start on the former approach in #55 (though not actually implemented the functionality yet!) but it would be easy enough to allow both options.

@brunns
Copy link
Contributor

brunns commented Aug 6, 2021

OK, #55 now implements regex matching of the messages, with a flag at the test level. Can someone let me know if I'm heading in the right direction? If so, I'll look at allowing regexes for specific messages, and also maybe the template idea.

sobolevn pushed a commit that referenced this issue Aug 9, 2021
* Some refactoring before making the change - use an object for expected output rather than a string.

* Simple regex case now working

* A spot of clean-up.

* Document regex option.

* Specify default value for regex flag.

* Add test for regexes inb out section.

* Add test to ensure that regexes ony run if the flag is switched on.

* Allow regexes on specific messages.

* Fix regex - single line flag froup needed to be optional.

* Add simple failing simple cases.

* Add case for mismatching regex.
@brunns
Copy link
Contributor

brunns commented Aug 10, 2021

Templates - would we want those at the level of a single test, or a single set of tests across the whole test file? I'm thinking the latter, given that the aim is to enable reuse.

@sobolevn
Copy link
Member

What templates are you talking about in this context? 🤔

@brunns
Copy link
Contributor

brunns commented Aug 10, 2021

It might also be possible to make the expected strings templates to enable reuse, and perhaps package some of the common patterns with the tool.

From the original issue report.

@sobolevn
Copy link
Member

sobolevn commented Aug 10, 2021

Oh, I've missed this part. What kind of common-case templates do you think we need?

@brunns
Copy link
Contributor

brunns commented Aug 10, 2021

I don't need them myself, but the OP's example included being able to specify a reveal template which expands to "Revealed type is", and another template which expands to the regex "(?:\*|`-?\d+)".

@brunns
Copy link
Contributor

brunns commented Aug 10, 2021

It was with this in mind that I did #57, since it would presumably use the same engine.

@brunns
Copy link
Contributor

brunns commented Aug 10, 2021

#58 contains a sketch of a solution. BTW, I believe that if templates at the individual test level is prefered, you can already do that using parametrized and a single set of values.

@brunns
Copy link
Contributor

brunns commented Sep 7, 2021

Are you planning a release, @sobolevn? I'd find this very useful over at https://github.com/hamcrest/PyHamcrest !

@sobolevn
Copy link
Member

sobolevn commented Sep 7, 2021

Yes, I will reelase it this week! Thanks again!

@sobolevn
Copy link
Member

sobolevn commented Sep 7, 2021

Done! Thanks a lot @brunns

@sobolevn sobolevn closed this as completed Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants