Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add golden testing #262

Merged
merged 17 commits into from
Nov 1, 2022
Merged

feat: add golden testing #262

merged 17 commits into from
Nov 1, 2022

Conversation

dandhlee
Copy link
Collaborator

@dandhlee dandhlee commented Oct 31, 2022

Adds goldens that I finally wanted to add!

Grabbed one of each so that I can easily take a look at the updated values across most libraries.

Removed a bunch of old artifacts for tests and on top directory. If it's distracting, I'll move it to another PR.

Unified tox unit test to a single tests. Added update-goldens session to be used locally if needed.

3.7 and 3.8 tests on GitHub Action seem to be causing weird discrepancies that I'm not sure why it's happening - sometimes it confuses "starting Line" and on other times just makes pure formatting issues that I'm not sure why it happens on GH actions. I was able to force this with 3.9 with sorting the TOC. I'll look into enabling it for 3.7 and 3.8 if needed.

Fixes #44

  • Tests pass
  • Appropriate changes to README are included in PR

@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Oct 31, 2022
@dandhlee dandhlee changed the title test: add golden testing feat: add golden testing Oct 31, 2022
@dandhlee dandhlee marked this pull request as ready for review October 31, 2022 12:19
@dandhlee dandhlee requested review from a team as code owners October 31, 2022 12:19

out_dir = build_dir / "html/docfx_yaml"

# Install dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't these installed normally? It would surprise me if a unit test installed something when I ran this locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mimics how Sphinx works in the repositories as well as for the plugin repo. It must have the packages installed before generating documentation. I could put this in tox.ini or in requirements.txt I think, but it'd be test data specific and thought it'd be better to keep it generic based on the given package.

Recommonmark is required in order to run sphinx-build with the extension. It seems that it's required within docuploader's shell command, so either I needed to add it here or add it in docuploader. Since this is specific to this repository's usage I found it more appropriate to install it here locally for test usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any time you use sphinx-docfx-yaml, you'll need Recommonmark, right? So, it seems reasonable to include it as a dependency, even if it's only directly used by this repo for tests? I may be misunderstanding.

Do you have a link to how docuploader's shell requires it (what is it?)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite - it's used for the extension but not directly in sphinx-docfx-yaml. That's why I included it as dependencies on each doc generation script (example: https://github.com/googleapis/python-logging/blob/77e621cb85b726a801227da85c31319f25969d19/noxfile.py#L312). When I tried including this in requirements.txt or tox.ini as part of dependency, docuploader.shell has trouble finding recommonmark required to run sphinx-build command and thus that's why I included it as a dependency.

# Settings to be used for sphinx-build
"-D",
# Extensions to be added
("extensions=sphinx.ext.autodoc,sphinx.ext.autosummary,docfx_yaml.extension," +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the + is needed at the end?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Did a bit of formatting as well.


if update_goldens:
shutil.rmtree(golden_dir, ignore_errors=True)
files_to_move = [file for file in out_dir.rglob("*")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the comprehension?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need! Updated.

@dandhlee
Copy link
Collaborator Author

Please take a look again!

@dandhlee dandhlee requested a review from tbpg October 31, 2022 19:58
# Place the Overview page at the top of the list.
app.env.markdown_pages.insert(
0,
index_page_entry,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is no index page entry?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully that's not the case - that means that the README page from GitHub is missing. However, I've added an if clause to check and return early if not found.


out_dir = build_dir / "html/docfx_yaml"

# Install dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any time you use sphinx-docfx-yaml, you'll need Recommonmark, right? So, it seems reasonable to include it as a dependency, even if it's only directly used by this repo for tests? I may be misunderstanding.

Do you have a link to how docuploader's shell requires it (what is it?)?

@dandhlee dandhlee merged commit ff8902e into main Nov 1, 2022
@dandhlee dandhlee deleted the add_goldens branch November 1, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve testing infrastructure to verify the output
2 participants