search

regro / regolith

Research Group Content Managment System
http://regro.github.io/regolith-docs/
Other
15 stars 71 forks source link

fix flake8 test_helpers.py #1188

Closed tinatn29 closed 3 days ago

tinatn29 commented 2 weeks ago

fix #1184 (lines too long)

bobleesj commented 2 weeks ago

fix issue #1184 (lines too long)

Thanks Tina, to automatically close the issue, could you please check the link below?

I wasn't sure whether fix issue ... works.

Ref: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

tinatn29 commented 2 weeks ago

Can we merge this one into cookie? (or let me know if there's anything to fix here!) Do we manually close #1184 or will we close once we merge cookie into main?

tinatn29 commented 2 weeks ago

actually it might be better to not merge this one yet? I'm not 100% sure that during this fix I had kept the same amount of trailing spaces, which might make tests (perhaps unnecessary) fail?

(On a different branch I'm dealing with failing tests right now with Assertion errors, where expected and actual outputs are the same texts but with maybe some spacing differences...)

sbillinge commented 1 week ago

OK, I will wait to merge this.

sbillinge commented 1 week ago

@tinatn29 please can you give an update on where we are with the cookiecut? I would like to get the tests passing, but when I checked, they still have not been moved over to regolith/tests. Are there other biggish changes that still need to be done.

Ideally we would have gotten the tests working before too much of the cookiecutting to avoid having to merge too much stuff with failing tests. Most of the cookiecutting doesn't break things, but it is just more confidence inspiring if tests are passing after each step of teh cookie cutting rather than waiting until the end....

tinatn29 commented 4 days ago

Sorry for the delay! I've just fixed the failed tests (when running pytest locally). I'll move the tests right after you merge #1198 into cookie. Right now the tests show 34 skipped and 57 warnings. I'll fixed these after moving the tests

sbillinge commented 4 days ago

I merged #1198 . Does this PR now need to be closed or merged? I am nervous updating test outputs

tinatn29 commented 4 days ago

So am I. My vote is we close this without merging and make pre-commit flake8 ignore test_helpers.py so we don't mess up the tests. What do you think @bobleesj?

bobleesj commented 4 days ago

So am I. My vote is we close this without merging and make pre-commit flake8 ignore test_helpers.py so we don't mess up the tests. What do you think @bobleesj?

Yeah, changing the test output without having CI configured initially is usually a scary idea since we don't know the time it has actually worked. I believe this is a pre-commit problem not pytest problem? @tinatn29 is the CI test running now?

tinatn29 commented 4 days ago

So am I. My vote is we close this without merging and make pre-commit flake8 ignore test_helpers.py so we don't mess up the tests. What do you think @bobleesj?

Yeah, changing the test output without having CI configured initially is usually a scary idea since we don't know the time it has actually worked. I believe this is a pre-commit problem not pytest problem? @tinatn29 is the CI test running now?

If what you mean by CI test is the same thing as passing when running pytest locally -- then yes (tests are passing with some skips and warnings)! see new PR #1199. Let's close this one and merge #1199?

sbillinge commented 3 days ago

So am I. My vote is we close this without merging and make pre-commit flake8 ignore test_helpers.py so we don't mess up the tests. What do you think @bobleesj?

Yeah, changing the test output without having CI configured initially is usually a scary idea since we don't know the time it has actually worked. I believe this is a pre-commit problem not pytest problem? @tinatn29 is the CI test running now?

If what you mean by CI test is the same thing as passing when running pytest locally -- then yes (tests are passing with some skips and warnings)! see new PR #1199. Let's close this one and merge #1199?

I think that for @bobleesj and I it might be better to work to get the CI working First. It is good that you have tests passing locally, and if so our CI could be you copy pasting a screenshot of the outcome, but maybe the top priority now is doing the work to get the CI working. @bobleesj we will run into the gooey issue again here.