Custom assertions by ggorlen · Pull Request #7 · codewars/python-test-framework · GitHub
Skip to content

Custom assertions#7

Closed
ggorlen wants to merge 10 commits intocodewars:masterfrom
ggorlen:custom-assertions
Closed

Custom assertions#7
ggorlen wants to merge 10 commits intocodewars:masterfrom
ggorlen:custom-assertions

Conversation

@ggorlen
Copy link
Copy Markdown
Contributor

@ggorlen ggorlen commented Jun 19, 2020

No description provided.

@ggorlen
Copy link
Copy Markdown
Contributor Author

ggorlen commented Jun 20, 2020

@kazk
Copy link
Copy Markdown
Member

kazk commented Jun 20, 2020

Yeah, it's difficult because of the original design.

If we can start over, we can simplify all this to:

  • <PASSED::> unless something throws
  • catch AssertionError for <FAILED::>
  • otherwise <ERROR::>

(or just use established test frameworks!)

@ggorlen
Copy link
Copy Markdown
Contributor Author

ggorlen commented Jun 20, 2020

Yeah, it's not hard to do this with a bit of redesign and keeping state differently. Just give a holler if you want me to work on it. I'm sure there's a good reason for it, but I guess unittest has to be adapted to work with the runner so that the proper stdout can be printed?

@kazk
Copy link
Copy Markdown
Member

kazk commented Jun 20, 2020

I'm sure there's a good reason for it

Do you mean the reason behind the original design? I don't think there was any "design" and that's the problem. If I remember correctly, Codewars test framework was created for JavaScript in the early days and it was decided that other languages should also have the similar API so custom frameworks for Ruby and Python were also written in the same way.
The only reason we're keeping it is backwards compatibility.

I guess unittest has to be adapted to work with the runner so that the proper stdout can be printed?

I'm not sure how it can be adapted, but I'm interested if you have some idea in mind.

I was thinking maybe we can change how the assertions work only when it's used inside it. That should allow us to switch to the simplified flow I wrote above without affecting the tests written without describe/it, unless I'm missing something.

@ggorlen
Copy link
Copy Markdown
Contributor Author

ggorlen commented Jun 20, 2020

I don't have an idea for making unittest work with the necessary CW stdout elements but I figured there was some trick that Q uses for this and could be adapted for CW as well.

Yeah, the simplified flow is promising but I think it'd take a bit of a redesign that felt invasive to propose. The current approach prints PASSED tags explicitly for each assertion rather than a single PASSED for the entire block when no exception is raised, so is this behavior acceptable? A lot of CW users have one it block (especially for random tests) and then run hundreds of assertions in it that each get a pass/no pass mark. Assuming I'm understanding correctly!

@ggorlen ggorlen closed this Jun 20, 2020
@kazk
Copy link
Copy Markdown
Member

kazk commented Jun 20, 2020

I figured there was some trick that Q uses for this and could be adapted for CW as well.

No, Q just uses unittest with custom runner/reporter. Qualified Test Framework == Codewars Test Framework and we've been discouraging our customers from using it.

The current approach prints PASSED tags explicitly for each assertion rather than a single PASSED for the entire block when no exception is raised, so is this behavior acceptable? A lot of CW users have one it block (especially for random tests) and then run hundreds of assertions in it that each get a pass/no pass mark.

I think changing to a single <PASSED::> per test case is acceptable. Most of the test frameworks work that way. If the authors wants to provide more information, they can still output some custom messages in between assertions. Or maybe we can make it configurable somehow.

@kazk
Copy link
Copy Markdown
Member

kazk commented Jun 20, 2020

Let's discuss and do small experiments before actually working on it.

Also, we should have some tests set up. Something like:

.
├── codewars_test
│   ├── __init__.py
│   └── test_framework.py
├── tests
│   ├── fixtures
│   │   ├── inside_it.expected.txt
│   │   ├── inside_it.py
│   │   ├── toplevel_assertions.expected.txt
│   │   └── toplevel_assertions.py
│   └── test_output.py
└── setup.py

And compare the actual output against the expected after removing things that's expected to be different like durations.

@ggorlen
Copy link
Copy Markdown
Contributor Author

ggorlen commented Jun 20, 2020

I like the single PASSED per test, and in fact this might discourage CW users from putting all of their assertions into one it. It does change functionality though, but hopefully in a non-harmful way.

On tests, yes--I was thinking of this when working on the repo.

Right, that makes sense for unittest, but I'm still not quite understanding why the same strategy isn't available for CW--backwards compatibility locks in the custom library, though.

@kazk
Copy link
Copy Markdown
Member

kazk commented Jun 20, 2020

Can you elaborate on the same strategy? I don't think I'm following. Qualified have the same problem.

@ggorlen
Copy link
Copy Markdown
Contributor Author

ggorlen commented Jun 20, 2020

Isn't it possible to use both unittest with a customer reporter alongside CW/Q unit tests? I know there is a dropdown option, but I'm assuming ultimately they have to emit the same stdout tags that the runner needs to display the results, so I assume that it's possible to plug n play different test libraries with the runner, as long as they ultimately generate the right standard output.

@kazk
Copy link
Copy Markdown
Member

kazk commented Jun 20, 2020

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.

2 participants