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

Refactored test_build.py #565

Merged
merged 2 commits into from
Nov 9, 2023
Merged

Conversation

mdellweg
Copy link
Contributor

@mdellweg mdellweg commented Nov 7, 2023

Made use of more generic with_[git_]project decorators.

Description

Checklist

  • Make sure changes are covered by existing or new tests.
  • For at least one Python version, make sure local test run is green.
  • Create a file in src/towncrier/newsfragments/. Describe your
    change and include important information. Your change will be included in the public release notes.
  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
  • Ensure docs/tutorial.rst is still up-to-date.
  • If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
  • If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

@mdellweg mdellweg force-pushed the refactor_build_tests branch 3 times, most recently from a7820ac to 875df57 Compare November 8, 2023 09:05
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Looks good. Only minor comments. Much appreciated.

I did a quick review between a meeting and a windows server core image build :)

f.write("**Blah blah**")

result = runner.invoke(command, ["--draft", "--date", "01-01-2001"])
@with_project()
Copy link
Member

Choose a reason for hiding this comment

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

happy to see the reduction in code indentation/nesting :)

@@ -157,7 +152,12 @@ def test_in_different_dir_config_option(self, runner):
self.assertEqual(0, result.exit_code)
self.assertTrue((project_dir / "NEWS.rst").exists())

@with_isolated_runner
@with_project(
Copy link
Member

Choose a reason for hiding this comment

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

this is so much easier to read. thanks a lot!

"""
foo 7.8.9 (01-01-2001)
======================
dedent(
Copy link
Member

@adiroiban adiroiban Nov 8, 2023

Choose a reason for hiding this comment

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

just asking. Was this change needed? was the test failing with the old code?

For assertion, I prefer to have as little processing as possible.

But we can also have this code... no problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not specifically needed. But this way it aligns better with the other tests doing assertions the same way.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. We can merge this. No problem.

Made use of more generic with_[git_]project decorators.

Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@mdellweg mdellweg marked this pull request as ready for review November 9, 2023 10:23
@mdellweg mdellweg requested a review from a team as a code owner November 9, 2023 10:23
@adiroiban
Copy link
Member

It looks good. I don't have time to look into the details... but I think that we are fine.
It's better than we have in trunk, the tests pass, so it shoudl be good to merge... the bar is low, I know, but this is what we can do now.

@adiroiban adiroiban merged commit d871ea3 into twisted:trunk Nov 9, 2023
15 checks passed
@mdellweg mdellweg deleted the refactor_build_tests branch November 9, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants