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

Fix for #54 - allow regexes when matching expected message text #55

Merged
merged 11 commits into from
Aug 9, 2021

Conversation

brunns
Copy link
Contributor

@brunns brunns commented Aug 6, 2021

No description provided.

@brunns brunns force-pushed the regex-message-assertions branch from 3518b79 to ced9be4 Compare August 6, 2021 13:02
README.md Outdated
@@ -62,6 +62,7 @@ On top of that, each case must comply to following types:
| `env` | `Optional[Dict[str, str]]={}` | Environmental variables to be provided inside of test run |
| `parametrized` | `Optional[List[Parameter]]=[]`\* | List of parameters, similar to [`@pytest.mark.parametrize`](https://docs.pytest.org/en/stable/parametrize.html) |
| `skip` | `str` | Expression evaluated with following globals set: `sys`, `os`, `pytest` and `platform` |
| `regex` | `str` | Allow regular expressions in comments to be matched against actual output. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say that the default is no

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed default to "no" - I'll mention that.

@brunns brunns force-pushed the regex-message-assertions branch from c239a2d to 9811300 Compare August 6, 2021 14:22
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thank you! Is it really still "WIP"?

Please, also check out #45
Maybe it is related / can be easily added.

@@ -6,3 +6,11 @@

reveal_type(a) # N: Revealed type is "builtins.int"
reveal_type(b) # N: .*str.*

- case: expected_message_regex_with_out
Copy link
Member

@sobolevn sobolevn Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need a test without regex attribute and with regex: no where we try to use regex output

Copy link
Contributor Author

@brunns brunns Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do negative tests - ie tests where we expect a failure. Are there any examples of this kind of test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if I added this test:

- case: should_fail
  regex: no
  main: |
    a = 'hello'
    reveal_type(a)  # N: .*str.*

I'd expect the test to fail - but that fails the build. Is there a way to express that I expect this to fail?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, see #46

The easiest way for now is to add all failing tests into a single non-collectable file. And run it with pytest after the main test case. We can grep that failed X tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you approve me to run workflows? I have something which works locally, but I'd like to see if it works in CI.

@brunns
Copy link
Contributor Author

brunns commented Aug 9, 2021

Is it really still "WIP"?

I suppose it could be merged now, but there are a couple of things we might want to add. Firstly, the template idea from #54, and secondly, some way of making a single expectation a regex rather than the whole test. What do you think?

Re #45, I think this PR works for that use case, though in a slightly different way.

@sobolevn
Copy link
Member

sobolevn commented Aug 9, 2021

a single expectation a regex rather than the whole test

Maybe we can use NR: .*regex.*, where R suffix means regex?

@brunns brunns marked this pull request as ready for review August 9, 2021 09:53
@brunns brunns force-pushed the regex-message-assertions branch from 83a56e1 to 8defa6a Compare August 9, 2021 09:54
@brunns
Copy link
Contributor Author

brunns commented Aug 9, 2021

I'm not sure the template idea belongs in this PR, so I think it's ready to go AFAIC.

@brunns brunns force-pushed the regex-message-assertions branch from 9acfd71 to 848282e Compare August 9, 2021 10:19
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! One last test and we are good to go!

Thanks a lot for your time and effort! 👍

a = 'hello'
reveal_type(a) # N: .*str.*

- case: rexex_but_turned_off
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add one more test that should fail: with wrong regex. Something that does not match the output 👍

@sobolevn sobolevn merged commit 170b21d into typeddjango:master Aug 9, 2021
@sobolevn
Copy link
Member

sobolevn commented Aug 9, 2021

Thanks a lot for your help!

@brunns brunns deleted the regex-message-assertions branch August 9, 2021 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants