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

docs: add "how to run tests" to developer.md #7787

Closed
wants to merge 1 commit into from

Conversation

TheJoeSchr
Copy link
Contributor

Summary

Adding some hinters to docs about how to run tests for first time

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.275% when pulling 4d6c52b on TheJoeSchr:develop into 5a489ce on freqtrade:develop.

@@ -49,6 +49,13 @@ For more information about the [Remote container extension](https://code.visuals
New code should be covered by basic unittests. Depending on the complexity of the feature, Reviewers may request more in-depth unittests.
If necessary, the Freqtrade team can assist and give guidance with writing good tests (however please don't expect anyone to write the tests for you).

#### How to run tests

Use `py.test` in root folder to run all available testcases and confirm your local environment is setup correctly
Copy link
Member

Choose a reason for hiding this comment

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

py.test is old terminology - pytest would be correct - and in the meantime probably depecated.

I'm also not entirely sure if we need this here as well - this document does link to the contributing guidelines - which do have detailed instructions on how to run pytest with freqtrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see py.test more as a standin for whatever is actually used. which I didn't know, because it's not documented anywhere in the docs, the first thing I was looking.

my experience today: I was trying to add a new feature, so I looked into the docs to see how tests are run for this project and I couldn't find it. so a simple, "hey this is how you get the ball rolling. having that hidden in another documents is not obvious. it would be better to just make a section. Tests: where are they, how to run them, what to expect and then link to the contributing guidelines for detailed instructions. But it should be stated somewhere, in the docs, how to tests this project from a clean fork

Copy link
Member

@xmatthias xmatthias Nov 22, 2022

Choose a reason for hiding this comment

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

That's great - as that's exactly how it is now if you read through the developer setup section carefully.

it explains the tools that will be installed and are used as part of development (pytest, mypy, flake8 and coveralls) - with the assumption of developers as audience -> audience knows how to use them, otherwise that's a research project on it's own.

it's then linking to the contributing guidelines for detailed instructions on HOW to use them.

@TheJoeSchr
Copy link
Contributor Author

TheJoeSchr commented Nov 23, 2022 via email

@xmatthias
Copy link
Member

xmatthias commented Nov 23, 2022

I'm sorry you see this as "fighting hard".
I've asked a question as to "why do we need this" - as i think it's covered.

The (rendered) documentation is there mostly for Users. it's common convention for open source projects to have a Contributing.md document which outlines actual details developers.

you described your experience - and i told you that apparently, you didn't read the developer section of the documentation in full / follow it's references.
Your reasoning is "it's not there" - which is clearly not true.
That doesn't mean it can't be improved - so i don't really see why you're becoming defensive.

I've mentioned that the command used is deprecated (which is a request for change).
I'll happily consider it once that's been changed. If i see a PR as "won't accept anyway" - i will outright close the PR (or at least mention that it's unlikely to be merged if i see the need for discussion - but see it as unlikely at that point).

The "note" part is wrong in other regards (tests are expected to pass on both the stable and develop branch) - but I'm more than happy to ignore that, as you'll have seen, i didn't even mention that in the review above.

The problem goes deeper than this though - which is something i'll say upfront - we will not cover as part of documentation.

Running/writing tests is either something you're familiar with - alternatively you'll struggle with them anyway and WILL need to do some research/learning upfront - outside of the freqtrade context - as there's a few concepts that - if unfamiliar - make them quite difficult to use (most prevalent example is mocking).

@TheJoeSchr
Copy link
Contributor Author

Ok, good you clarified. Sometimes it feels I spent a lot more time arguing than implementing an fix.

I think we just have different perspective. I come from a beginners mind. A beginner doesn't have to be somebody that isn't a developer, it can also be somebody new to a language or framework

Running/writing tests is either something you're familiar with - alternatively you'll struggle with them anyway and WILL need to do some research/learning upfront - outside of the freqtrade context - as there's a few concepts that - if unfamiliar - make them quite difficult to use (most prevalent example is mocking).

I for example have been programming python for more than a decade now and have been using test driven development for years but I have never done them together.

You have been working on freqtrade far longer. So of course things that seem obvious to you are not to other people with a different experience.

I'm arguing for these people, that's why I meant, keep beginners mind in mind. In my opinion it's also one of the most valuable resources if you want to attract contribution. recording the experiences of somebody, coming from all walks of life, who wants to try to engage with a task and see how they might do it and where their experiences starts to fail & gets roadblock.

So my report is: I wanted to do TDD but I couldn't even find how tests should be run for this project by looking at the obvious place: the docs.

Many others might have left at this point and we most likely wouldn't have heard of them.

But since I learned about beginners mind I made a habit out of recording this experiences and trying to at least instigate change for the better. This is this PR.

About the exact content, as I think I made clear by now, I don't know how test should be run for the first time.

So please add or change everything of my PR as needed so people like me know how you expect others to run tests and verify everything is set up as expected before they start their tests or run pre-commit install. Like I said, even asking around, I got different answers. So it's good to document a common ground at an resources most likely to be looked at. Many people may be ignorant of an contribution.md. Others might not be thinking about themselves as contributers, but just wanted to get into the thick of it and run test first to see if dev setup worked. There are many reasons to place a basic "Tests: Where to find, how to run, what to expect" at an prominent position at the resources you expect people to look first (aka manual as in RTFM)

I think it will lead to a better developer and beginners experience

@xmatthias
Copy link
Member

Merged directly in a85602e and 320535a (i was too stupid to actually do the merge properly).

@xmatthias xmatthias closed this Nov 27, 2022
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