Refactor/new docstrings styling

[Dioprz]

Description

After three weeks researching about alternatives to run doctests dinamically, I finally found how to put the pieces together. The result is this PR where we have:

• The implementation of the first doctests migration from Sagemath to Python.
• The changes proposed here Refactor Docstring Style to Google Style #162, to change the docstrings style into a more standard approach (Google docstrings).
• The creation of a pytest CLI option, so we can trigger the dynamic definition of a boolean flag via pytest fixtures.
• A new docstrings logic to dinamically decide if we run or not long doctests.

I made the changes in just one file so we can discuss any aspect, before go all-in.

How this changes work

At conftest.py

1. We now have a pytest fixture called automatically on each pytest CLI execution, as well as a new pytest CLI option called --long-doctests, that defaults to False but changes to True when provided.
2. Whatever value we get from this CLI option, we put it into a long_doctests variable that is later passed to the "context" of our doctests via doctest_namespaces fixtures
   

At sd_estimator.py

We now have to collect all the tests that could take a long time to run, and put them at the end of the Tests: section. This way, we can use the long_doctests flag comming from the pytest context, and with the conditional

>>> if not long_doctests:
 ...     pytest.skip()

Ensure that every test block after that will be run only if the flag long_doctests is True (i.e., if we made the pytest call with the --long-doctests option).

Pros and drawbacks

Pros

1. The #long time functionality of Sage markers is, essentially, preserved.

Drawbacks

1. Now we don't have the granularity of a docstring marker, so we can't alternate between long and short time blocks inside a docstring. Every long test should be at the end of the Tests section.
2. When the --long-doctests option is not provided to the pytest call, all the tests containing long test are reported as skipped instead of passed. While this shouldn't have any technical implication, because short failing tests still produces a pytest error, I think it's worth to mention it.

Review process

• For short tests/examples:
  • Run pytest --doctest-modules cryptographic_estimators/SDEstimator/sd_estimator.py in the shell. It will work, and the pytest return will be 1 skipped.
  • Change some value of the expected output for the Examples: section of the docstring, and save the file. Run the same command again and, as expected, pytest will return 1 failed.
• For long tests:
  • Run pytest --long-doctests --doctest-modules cryptographic_estimators/SDEstimator/sd_estimator.py. Long tests defined after the previousd conditional, will be executed and the pytest call will return 1 passed.

Pre-approval checklist

• The code builds clean without any errors or warnings
• I've added/updated tests
• I've added/updated documentation if needed

[Dioprz]

I made one change to the name of the flag from long-doctests to skip-long-doctests, so it is easier to understand. The flag usage is inverted because of this:

### Previous
### >>> if not long_doctests:
### ...     pytest.skip()

### New
>>> if skip_long_doctests:
...     pytest.skip()

And the testing process looks now like this

1. make docker-build
2. make docker-run
3. pytest --doctest-modules cryptographic_estimators/SDEstimator/sd_estimator.py -> Produces as output 1 passed
4. pytest --skip-long-doctests --doctest-modules cryptographic_estimators/SDEstimator/sd_estimator.py -> Produces as output 1 skipped
5. You can change any value in the Examples: block to test the short execution failure (remember to rebuild the docker with your change).
6. You can change any value in the Examples: or Tests: blocks to test the long execution failure.

It would be great if this PR could have at least three approvals as it redefines a new standard for the library. Please let me know if there are any objections.

[Dioprz]

As shown in ad0b9d1, a problem emerges with the CI when changing the docstring style.

The problem: Sage collects not only doctests with the Sage format, but also doctests with the Python format. Because the new format uses a feature that we only want to execute (and can provide) with Pytest and pure Python (the --skip-long-doctests, that feeds the skip_long_doctests fixture used in the new test style), the doctest fails.

The solution idea:

We need to be able to:

1. Introduce the style changes in the doctests of some file X.
2. Exclude X from being collected by the Sage doctest run process.
3. Include X into the collection of Pytest.

This way, we will make a safe testing transition in each PR, as in any state of the repo, every doctest will be evaluated by either Sage or Python, but not both.

Solution implementation:

• At 5f2b143, I made two changes:

  1. Create a new command docker-pytest¹. This command executes Pytest with pure Python, and declares every subdirectory and file inside cryptographic_estimators/, plus the tests/ directory.
  2. Refactor the commands docker-test and docker-testfast so they include every subdirectory inside cryptographic_estimators/.

  While the three commands declare the same subdirectories, being extremely verbose, you can see too that each line commented in docker-test[fast] is not commented in docker-pytest. The intention behind this is to have the next workflow during the docstring transition:

  1. We will have just one docstring-migration PR per estimator. Each one of those PRs will apply the new style to every file inside the estimator subdirectory.
  2. We comment the estimator directory in docker-test and docker-testfast, and uncomment it in docker-pytest.
  3. The transition is done for that estimator, and we can move forward with the next one.
  4. At the end, the docker-test[fast] command will disappear, and we can remove the explicit directory listing from the pytest command.

• At 238141b, I include the make docker-pytest command into our CI.

Footnotes

1. We already had one docker-pytest command declared. However, and as you can see in the comment of 5f2b143, such command wasn't being used in any GH action (you can confirm with grep -r "docker-pytest" .github). For that reason, and because that command wasn't correctly structured, I decided to take its name and re-implement it. ↩

[FloydZ]

Looks very good.
Cool approach for splitting the tests for sage and python.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[Memphisd]

Looks good to me.