8000 8000
Skip to content

Refactor tests to remove repetition.#833

Open
NDevox wants to merge 1 commit intoflask-restful:master 8000 from
NDevox:master
Open

Refactor tests to remove repetition.#833
NDevox wants to merge 1 commit intoflask-restful:masterfrom
NDevox:master

Conversation

@NDevox
Copy link
Copy Markdown
@NDevox NDevox commented Oct 19, 2019

This is a superficial refactor of the tests to be a bit more streamlined
by removing obvious repetition from the test classes. Most of this is
done by simply adding a Flask app instance with an API in the setUp
method of the TestCase classes. The tests in terms of what they do have
been unchanged.

This provides slightly simpler code to read - and provides new tests
with pre setup app and api instances if needed.

Also a minor refactor of import orderings in tests to match PEP8.

This was done as a colleague of mine had been building unit tests based off of the ones in flask-restful, leading to a conversation about reducing repetition in tests and making them more readable.

I note there was also a PR to move to pytest (#704) which was rejected. Considering nose has been deprecated for years and that pytest generally results in smaller and simpler tests, is there a reason it was rejected?

I'd be happy to redo this PR with pytest as an example.

This is a superficial refactor of the tests to be a bit more streamlined
by removing obvious repetition from the test classes. Most of this is
done by simply adding a Flask app instance with an API in the setUp
method of the TestCase classes. The tests in terms of what they do have
been unchanged.

This provides slightly simpler code to read - and provides new tests
with pre setup app and api instances if needed.

Also a minor refactor of import orderings in tests to match PEP8.
@NDevox
Copy link
Copy Markdown
Author
NDevox commented Oct 19, 2019

Looks like the coverage decreased because the total volume of the test code decreased :-D

@joshfriend
Copy link
Copy Markdown
Member

Thank you :)

Test code is not included in coverage results, so somehow, this change did decrease coverage. I am not sure why. Or maybe coveralls is lying 😆I don't know which yet.

I note there was also a PR to move to pytest which was rejected.

Yeah it changed so much stuff and there was still active development at the time and the merge conflicts weren't worth it. Personally I like pytest a lot, but I don't have

@NDevox
Copy link
Copy Markdown
Author
NDevox commented Oct 20, 2019

Damn I was hoping for an easy explanation 🤔.

Well if you're happy with pytest and development has slowed down I can get a pytest PR out and move to functional tests instead of class based.

Maybe it'll sort out the coverage issues.

@NDevox
Copy link
Copy Markdown
Author
NDevox commented Oct 20, 2019

@joshfriend see this PR: #834

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

0