Refactor CI

[JoaoRodrigues]

Changes to workflow:

• Replace CodeCov by simple coverage report/check
• Build/Coverage for every OS/Python version.
• Try and reinstate windows testing.

[JoaoRodrigues]

Some more changes:

• Added unittest.skipIf to tests requiring a TTY, so that they are skipped on windows which is where we cannot have a workaround.

[JoaoRodrigues]

@joaomcteixeira, have a look and give your opinion. I tried making this a consensus of our opinions :)

[JoaoRodrigues]

By the way, the workflows were not running because of a syntax issue with YAML. So, I added a separate YAML linter to detect these cases. It's a bit of a chicken and egg problem (since the workflow to detect yaml issues is written in yaml) but since we start from a 'clean' slate of working yaml files, it should be easy to catch future failures.

[joaomcteixeira]

• I don't mind having the report sent to Codecov. I think it facilitates newcomers (and us) to visualize the coverage rate quickly. It also integrates with the badge in the readme. However, there is no real need for it, as we can see the coverage rates in the GitHub action, though it requires some clicks and knowledge. As you said, running CIs everywhere has its energy consumption penalty, but running another build also does. Honestly, I don't know which is more energy efficient: sending the coverage reports to codecov after each testing or merging them at the end in the Github actions. I am OK with either way here. If we drop Codecov, also remove the badge in the README.

• The Yaml linter is chicken and egg, indeed. Usually, actions won't trigger if there's a syntax error in the YAML. But still, it is a good addition.

• I would add a small comment in the setup.cfg or in the CONTRIBUTING with the commands developers need to run locally for flake8 and coverage. So developers can access lint results and the coverage table before PR. Before, we were not checking lint in the CIs, but if this PR is accepted, we will; so developers need to have local access to it.

• Regarding skipping tests on windows. I suggest using a global variable instead of ignoring the tests when sys.platform.startswith('win'). I suggest we define a global variable in the YAML CI and then @unittest.skipIf("VARIABLE" in os.environ). In this way, WIN users would have access to all tests locally, and tests would only be skipped in the Actions.

[JoaoRodrigues]

I don't mind having the report sent to Codecov. I think it facilitates newcomers (and us) to visualize the coverage rate quickly. It also integrates with the badge in the readme. However, there is no real need for it, as we can see the coverage rates in the GitHub action, though it requires some clicks and knowledge. As you said, running CIs everywhere has its energy consumption penalty, but running another build also does. Honestly, I don't know which is more energy efficient: sending the coverage reports to codecov after each testing or merging them at the end in the Github actions. I am OK with either way here. If we drop Codecov, also remove the badge in the README.

👍

I would add a small comment in the setup.cfg or in the CONTRIBUTING with the commands developers need to run locally for flake8 and coverage. So developers can access lint results and the coverage table before PR. Before, we were not checking lint in the CIs, but if this PR is accepted, we will; so developers need to have local access to it.

Noted, will update the CONTRIBUTING file.

Regarding skipping tests on windows. I suggest using a global variable instead of ignoring the tests when sys.platform.startswith('win'). I suggest we define a global variable in the YAML CI and then @unittest.skipIf("VARIABLE" in os.environ). In this way, WIN users would have access to all tests locally, and tests would only be skipped in the Actions.

Good point. I can have a check for a specific env variable instead.

[joaomcteixeira]

Great work! Some changes are still needed:

[JoaoRodrigues]

OK, finally got it to work. The workflow now skips tests that fail without a TTY specifically on MacOS and Windows, by setting an env var SKIP_TTY_TESTS if the runner OS is not Linux. Also changed the readme to remove the codecov badge (added a badge from pypi to indicate downloads/month instead, so it's not very empty :)), and the contributing guidelines (that already mentioned flake8 but you never read it I guess :P).

[joaomcteixeira]

Perfect end result! 👍 really nice. 👏 after so many commits ... feels good