feat(anta): Add Markdown report option to ANTA #740

carl-baillargeon · 2024-07-04T20:02:48Z

Description

Added _results_per_status as a cached property to fetch results faster.

anta nrfu md-report --md-output ./anta_md_report.md
anta nrfu md-report --md-output ./anta_md_report.md --only_failed_tests

Fixes # (issue id)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have run pre-commit for code linting and typing (pre-commit run)
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes (tox -e testenv)

titom73 · 2024-07-05T07:49:06Z

We should discuss how we want to add new outputs to make it consistent across multiple PRs

Linked to PR #672

mtache · 2024-07-05T10:28:06Z

I don't think this PR requires #649 since it is not about refactoring the ANTA Runner.
You can still benchmark CLI output options if you want to improve their performances but this PR introduces a new format so nothing to be compared with.

github-actions · 2024-07-08T19:51:11Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-07-08T20:16:32Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

anta/result_manager/constants.py

anta/cli/nrfu/commands.py

gmuloc · 2024-07-10T12:31:01Z

anta/cli/nrfu/utils.py

@@ -122,6 +124,14 @@ def print_jinja(results: ResultManager, template: pathlib.Path, output: pathlib.
            file.write(report)


+def save_markdown_report(ctx: click.Context, md_output: pathlib.Path, *, only_failed_tests: bool = False) -> None:
+    """Save the markdown report."""


docstring need more info (Parameters)

Fixed in 30c8d7e

anta/reporter/md_reporter.py

anta/result_manager/__init__.py

gmuloc · 2024-07-10T15:53:45Z

tests/units/reporter/test_md_reporter.py

+)
+def test_md_report_generate(tmp_path: Path, expected_report_name: str, *, only_failed_tests: bool) -> None:
+    """Test the MDReportGenerator class."""
+    # Create a temporary Markdown file


it does not create the file right?

No. tmp_path is a pytest fixture --> https://docs.pytest.org/en/stable/how-to/tmp_path.html

so let's remove this comment

Fixed in fb2139a

tests/units/reporter/test_md_reporter.py

gmuloc · 2024-07-10T15:54:18Z

tests/units/reporter/test_md_reporter.py

+    # Load the existing Markdown report to compare with the generated one
+    with (DATA_DIR / expected_report_name).open("r", encoding="utf-8") as f:
+        expected_content = f.read()


lets make a smaller test set

What do you mean? Creating multiple tests for each section (class) of the report?

less tests I was thinking

Reduced the amount of tests in fb2139a

tests/units/result_manager/test__init__.py

gmuloc · 2024-07-12T11:41:02Z

anta/reporter/md_reporter.py

+        ----------
+            results: The ResultsManager instance containing all test results.
+            md_filename: The path to the markdown file to write the report into.
+            only_failed_tests: Flag to generate a report with only failed tests. Defaults to False.


Suggested change

only_failed_tests: Flag to generate a report with only failed tests. Defaults to False.

only_failed_tests: Flag to generate a report with only failed tests.

Are you suggesting to remove the default in the function signature as well?

no only in the docstring

Removed only_failed_tests in 8918cd0

gmuloc · 2024-07-12T11:42:57Z

anta/reporter/md_reporter.py

+        toc = """**Table of Contents:**
+
+- [ANTA Report](#anta-report)
+  - [Test Results Summary](#test-results-summary)
+    - [Summary Totals](#summary-totals)
+    - [Summary Totals Device Under Test](#summary-totals-device-under-test)
+    - [Summary Totals Per Category](#summary-totals-per-category)
+  - [Failed Test Results Summary](#failed-test-results-summary)"""


let's move this to a constant outside of here so that it does not look weird to look at with indent

Fixed in d217ac3

gmuloc · 2024-07-12T12:05:21Z

anta/result_manager/__init__.py

+        # Every time a new result is added, we need to clear the cached property
+        self.__dict__.pop("results_by_status", None)
+
+    def get_results(self, status: set[TestStatus] | TestStatus | None = None, sort_by: list[str] | None = None) -> list[TestResult]:


would it not be simpler for us to just support set[TestStatus] | None - I mean - who is the convenience method for? to support single TestStatus ?

get_results and get_total_results have the same signature and I think it's cleaner when we need a single status in md_reporter --> self.results.get_total_results('success')

I would change also get_total_results - I disagree it is cleaner to have signature with unions when we can avoid them as this means we need to handle the types in the code, though I agree it looks nicer to write.

Fixed in 8918cd0

anta/result_manager/__init__.py

gmuloc · 2024-07-12T12:08:53Z

anta/result_manager/models.py

+# pylint: disable=too-many-instance-attributes
+@dataclass
+class DeviceStats:
+    """Device statistics for a run of tests."""
+
+    tests_success_count: int = 0
+    tests_skipped_count: int = 0
+    tests_failure_count: int = 0
+    tests_error_count: int = 0
+    tests_unset_count: int = 0
+    tests_failure: set[str] = field(default_factory=set)
+    categories_failed: set[str] = field(default_factory=set)
+    categories_skipped: set[str] = field(default_factory=set)


what is a benefit of us using a mix of Pydantic BaseModel and datalclasses? (Not asking to change it just wondering if we are not kind of mix and matching things when we should 'stick to one')?

Pydantic will validate the fields, dataclasses will not by default. Since these classes are basically "storing" internal data (not user facing), we don't need to validate anything.

gmuloc · 2024-07-12T12:09:33Z

docs/cli/nrfu.md

+Usage: anta nrfu md-report [OPTIONS]
+
+  ANTA command to check network state with Markdown report. It only saves test
+  results and not the output from the --group-by option.


Suggested change

results and not the output from the --group-by option.

results and not the output from the `anta nrfu --group-by` option.

Actually I am confused now. --group-by is an option for the table command so why do we need to specify this?

Removed in 5c44994

gmuloc · 2024-07-12T12:12:25Z

tests/data/test_md_report_only_failed_tests.md

+| DC1-LEAF1A | BFD | VerifyBFDPeersIntervals | Verifies the timers of the IPv4 BFD peers in the specified VRF. | - | failure | Following BFD peers are not configured or timers are not correct: {'192.0.255.8': {'default': 'Not Configured'}, '192.0.255.7': {'default': 'Not Configured'}} |
+| DC1-LEAF1A | BFD | VerifyBFDSpecificPeers | Verifies the IPv4 BFD peer's sessions and remote disc in the specified VRF. | - | failure | Following BFD peers are not configured, status is not up or remote disc is zero: {'192.0.255.8': {'default': 'Not Configured'}, '192.0.255.7': {'default': 'Not Configured'}} |
+| DC1-LEAF1A | BGP | VerifyBGPAdvCommunities | Verifies the advertised communities of a BGP peer. | - | failure | Following BGP peers are not configured or advertised communities are not standard, extended, and large: {'bgp_peers': {'172.30.11.17': {'default': {'status': 'Not configured'}}, '172.30.11.21': {'default': {'status': 'Not configured'}}}} |
+| DC1-LEAF1A | BGP | VerifyBGPExchangedRoutes | Verifies the advertised and received routes of BGP peers. | - | failure | Following BGP peers are not found or routes are not exchanged properly: {'bgp_peers': {'172.30.255.5': {'default': 'Not configured'}, '172.30.255.1': {'default': 'Not configured'}}} |
+| DC1-LEAF1A | BGP | VerifyBGPPeerASNCap | Verifies the four octet asn capabilities of a BGP peer. | - | failure | Following BGP peer four octet asn capabilities are not found or not ok: {'bgp_peers': {'172.30.11.1': {'default': {'status': 'Not configured'}}}} |


well .. what it impacts is maintainability - if we deprecate tests for instance we need to come back here and modify this file - which is big and which we will probably forget

gmuloc · 2024-07-12T12:13:04Z

tests/units/reporter/test_md_reporter.py

+)
+def test_md_report_generate(tmp_path: Path, expected_report_name: str, *, only_failed_tests: bool) -> None:
+    """Test the MDReportGenerator class."""
+    # Create a temporary Markdown file


so let's remove this comment

gmuloc · 2024-07-12T12:13:34Z

tests/units/reporter/test_md_reporter.py

+    # Load the existing Markdown report to compare with the generated one
+    with (DATA_DIR / expected_report_name).open("r", encoding="utf-8") as f:
+        expected_content = f.read()


less tests I was thinking

gmuloc · 2024-07-17T08:34:23Z

anta/cli/nrfu/utils.py

+        only_failed_tests: If True, only failed tests will be included in the report. Default is False.
+    """
+    console.print()
+    MDReportGenerator.generate(results=_get_result_manager(ctx), md_filename=md_output, only_failed_tests=only_failed_tests)


This could fail and raise an OSError, in that case we would need to catch it cleanly and not print "Markdown report saved"

Fixed in 5c44994

gmuloc · 2024-07-17T08:34:50Z

anta/reporter/md_reporter.py

+        """
+        MDReportBase.ONLY_FAILED_TESTS = only_failed_tests
+
+        with md_filename.open("w", encoding="utf-8") as mdfile:


need to catch OSError here if anything goes wrong - cf #672

Fixed in 5c44994

github-actions · 2024-07-18T12:17:42Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-07-18T14:00:03Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

gmuloc · 2024-07-18T14:34:29Z

anta/cli/nrfu/commands.py

+@click.option(
+    "--only-failed-tests",
+    is_flag=True,
+    default=False,
+    show_envvar=True,
+    help="Only include failed tests in the report.",
+)


do we need this new option or could we not use the nrfu option

--hide [success|failure|error|skipped] Hide results by type: success / failure / error / skipped'.

instead and pass it down the line?

Agreed. Removed in 8918cd0

gmuloc · 2024-07-18T14:39:32Z

anta/reporter/md_reporter.py

+    The class method also accepts an optional `only_failed_tests` flag to generate a report with only failed tests.
+
+    By default, the report will include all test results.


lets use the --hide content instead for greater versatility

Fixed in 8918cd0

gmuloc · 2024-07-18T14:40:00Z

anta/reporter/md_reporter.py

+        ----------
+            results: The ResultsManager instance containing all test results.
+            md_filename: The path to the markdown file to write the report into.
+            only_failed_tests: Flag to generate a report with only failed tests. Defaults to False.


no only in the docstring

gmuloc · 2024-07-18T14:40:41Z

anta/reporter/md_reporter.py

+        -------
+            str: Formatted header name.
+
+        Example:


Suggested change

Example:

Example

Fixed in 8918cd0

gmuloc · 2024-07-18T14:41:08Z

anta/reporter/md_reporter.py

+        ----------
+            heading_level: The level of the heading (1-6).
+
+        Example:


Suggested change

Example:

Example

Fixed in 8918cd0

gmuloc · 2024-07-18T14:47:10Z

anta/result_manager/__init__.py

+        # Every time a new result is added, we need to clear the cached property
+        self.__dict__.pop("results_by_status", None)
+
+    def get_results(self, status: set[TestStatus] | TestStatus | None = None, sort_by: list[str] | None = None) -> list[TestResult]:


I would change also get_total_results - I disagree it is cleaner to have signature with unions when we can avoid them as this means we need to handle the types in the code, though I agree it looks nicer to write.

for more information, see https://pre-commit.ci

gmuloc · 2024-08-29T07:51:11Z

tests/units/cli/nrfu/test_commands.py

@@ -162,15 +162,6 @@ def test_anta_nrfu_md_report_all_tests(click_runner: CliRunner, tmp_path: Path)
    assert md_output.exists()


-def test_anta_nrfu_md_report_only_failed_tests(click_runner: CliRunner, tmp_path: Path) -> None:


should you not keep a test with --hide <> whatever?

gmuloc

just one comment on the tests otherwise LGTM

sonarqubecloud · 2024-08-29T12:03:44Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

gmuloc

tested in a lab

github-actions bot assigned carl-baillargeon Jul 4, 2024

carl-baillargeon mentioned this pull request Jul 4, 2024

feat: Reporting enchancements #717

Open

github-actions bot added the state: conflict label Jul 8, 2024

carl-baillargeon added 6 commits July 8, 2024 16:15

feat(anta): Add markdown report option to ANTA CLI

c5c517a

Update MDReportFactory

ef3a942

Make Py3.11 happy with f-string

7920718

Remove unnecessary only_failed_tests from MDReportBase

0c1e957

Added TestStats

cb35a83

Fix typos

733c159

carl-baillargeon force-pushed the feat/md_report branch from 504cff4 to 733c159 Compare July 8, 2024 20:16

github-actions bot removed the state: conflict label Jul 8, 2024

carl-baillargeon added 6 commits July 8, 2024 17:17

Fix pytest

aa2168f

Added unit tests for reporter and result_manager

e72b22a

Added unit tests for md_reporter

874467b

Fix unit tests for md_reporter

650b8e2

Remove license from test md reports

230bd0a

Fix line too long

8410064

titom73 reviewed Jul 9, 2024

View reviewed changes

anta/result_manager/constants.py Outdated Show resolved Hide resolved

carl-baillargeon added 3 commits July 9, 2024 09:58

Added more unit tests

1747500

Added documentation

f34b1fe

Remove test.md

596d029

titom73 self-requested a review July 9, 2024 15:25

titom73 added framework-enhancement New feature or request Anta CLI All things around CLI labels Jul 9, 2024

titom73 approved these changes Jul 9, 2024

View reviewed changes

gmuloc requested changes Jul 10, 2024

View reviewed changes

gmuloc reviewed Jul 12, 2024

View reviewed changes

gmuloc mentioned this pull request Jul 12, 2024

Feat: Add a deprecated decorator for any function and use it on the unused functions #754

Open

gmuloc reviewed Jul 17, 2024

View reviewed changes

github-actions bot added the state: conflict label Jul 18, 2024

Reduce number of tests in fake reports

fb2139a

gmuloc mentioned this pull request Jul 18, 2024

refactor(anta): Change TestStatus to be an Enum for coding clarity #758

Merged

6 tasks

carl-baillargeon added 2 commits July 18, 2024 09:51

Fix per review comments

d217ac3

Rebase with latest main

5c44994

github-actions bot removed the state: conflict label Jul 18, 2024

Remove unrelated changes

16ae28c

gmuloc reviewed Jul 18, 2024

View reviewed changes

carl-baillargeon mentioned this pull request Aug 13, 2024

Feat: New anta_workflow plugin using PyAVD aristanetworks/avd#4196

Open

19 tasks

Merge branch 'main' into feat/md_report

dc23513

carl-baillargeon force-pushed the feat/md_report branch from 7cb3f82 to dc23513 Compare August 19, 2024 13:53

Fix per review comments

8918cd0

carl-baillargeon requested a review from gmuloc August 19, 2024 15:35

carl-baillargeon and others added 3 commits August 19, 2024 19:43

Merge branch 'main' into feat/md_report

7abf73a

[pre-commit.ci] auto fixes from pre-commit.com hooks

83d8602

for more information, see https://pre-commit.ci

Merge branch 'main' into feat/md_report

21cb559

gmuloc reviewed Aug 29, 2024

View reviewed changes

gmuloc approved these changes Aug 29, 2024

View reviewed changes

Added unit test for --hide

8372826

gmuloc approved these changes Aug 29, 2024

View reviewed changes

gmuloc merged commit 7bb4560 into aristanetworks:main Aug 29, 2024
19 checks passed

github-actions bot added the rn: feat(anta) label Aug 29, 2024

	only_failed_tests: Flag to generate a report with only failed tests. Defaults to False.
	only_failed_tests: Flag to generate a report with only failed tests.

	results and not the output from the --group-by option.
	results and not the output from the `anta nrfu --group-by` option.

		The class method also accepts an optional `only_failed_tests` flag to generate a report with only failed tests.

		By default, the report will include all test results.

		@@ -162,15 +162,6 @@ def test_anta_nrfu_md_report_all_tests(click_runner: CliRunner, tmp_path: Path)
		assert md_output.exists()


		def test_anta_nrfu_md_report_only_failed_tests(click_runner: CliRunner, tmp_path: Path) -> None:

feat(anta): Add Markdown report option to ANTA #740

feat(anta): Add Markdown report option to ANTA #740

Conversation

carl-baillargeon commented Jul 4, 2024 • edited Loading

Description

Checklist:

titom73 commented Jul 5, 2024

mtache commented Jul 5, 2024

github-actions bot commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2024

github-actions bot commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmuloc left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 29, 2024

Quality Gate passed

gmuloc left a comment

Choose a reason for hiding this comment

carl-baillargeon commented Jul 4, 2024 •

edited

Loading