#349 merged with failing unit tests

[alexdunnjpl]

🐛 Describe the bug

#349 was merged despite failing unit tests

📜 To Reproduce

See relevant branch integration test report

🕵️ Expected behavior

Branch integration tests should pass

⚙️ Engineering Details

Suggested resolution

• either populate the relevant config fields in the config used during BITs, or disable the relevant tests when run in CI
• temporarily disable flake8 lint in BIT Github Action, as the flood of failures hide non-trivial BIT failures.

@jordanpadams let me know if you want me to tackle this, I know @ramesh-maddegoda has a lot going on atm.

[jordanpadams]

@alexdunnjpl can we fix this, as well as any of the other flake issues that are occurring so we can get this CI working correctly? We will not be able to tag a release until this can be built cleanly.

[alexdunnjpl]

@jordanpadams roger that, will sort it out today.

I'll skip these tests for CI and disable the lint, and open icebox'd tickets for each of them.

After that, for the linting, do you want the codebase brought into spec for the existing lint profile, or the lint profile adjusted to disable the new warnings, or is fixing it a lower priority than closing out other tickets?

[alexdunnjpl]

@jordanpadams what's the level of urgency on this?

To sort this out properly requires:

• disabling the "new" flake8 rules that are flooding the lint stage with errors
• disabling the add cognito (jwt bearer) authentication #349-related tests in CI
• untangling some confusing elements of the tox configuration (currently, tox lint runs all unit tests - working on splitting that out now but I have to learn how2tox first)
• adding a pre-commit hook that devs can use locally to lint on commit (which, having sorted the previous bit, will take a few seconds rather than a few minutes and actually be a feasible pre-commit hook

Basically I want to know if it can wait until next week or not - it's not a lot of work, mind.

[alexdunnjpl]

nvm, we're good

[jordanpadams]

@alexdunnjpl just wanted to verify, does any of your changes to the tox / flake8 rules above conflict with what we have here: https://github.com/NASA-PDS/template-repo-python

the premise here is we want to ensure we are consistent across python repos. if we need to change something here, I want to make sure this is agreed upon with @nutjob4life and a PR is created on the template repo to ensure we use that config moving forward (and probably need to rollout to existing python repos)

[alexdunnjpl]

@jordanpadams

The flake8 ignores as-is are a superset of the desirable flake8 ignores for this project, which is the template set plus W503 - see L163 here

Based on the comment there, it seems like W503 should be added to the template.

Regarding the others, they're not desirable to keep long-term, but the alternative until the underlying style issues are fixed is to have the tests fail, removing the ability of the tests to enforce good style for any new contributions. So adding the bunch of temporary ignores is the least-worst option here.

[nutjob4life]

W503 should be added to the template

Yes yes yes.

Human perception studies (amongst left-to-right readers) show that people tend to scan for significance on the left sides of things, making the so-called "F" pattern for reading and skimming. W503's placement of operators on the right is wrong-headed.

[alexdunnjpl]

Looking at what that rule actually is, MAAASSIVE +1

Will open a PR on the template repo for that now.