Skip to content

Reduce technical debt of code duplication in automated tests #69

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

Merged
merged 4 commits into from
May 20, 2025

Conversation

arrestle
Copy link
Contributor

@arrestle arrestle commented May 15, 2025

  1. Updates to remove duplicate code and share constants and variables.
  2. Updates to the Makefile to add help, allow local linting through local .tools folder, remove hardcoded AAP_HOST.

@arrestle arrestle marked this pull request as draft May 15, 2025 20:53
@arrestle arrestle force-pushed the AAP-45797-tech-debt branch from 8a8984d to 24ca7e8 Compare May 15, 2025 21:35
@arrestle arrestle force-pushed the AAP-45797-tech-debt branch 2 times, most recently from c8b3ebd to 4614cb8 Compare May 16, 2025 14:35
@arrestle arrestle changed the title makefile refactor for help. Cleanup. Seeing a panic Reduce technical debt of code duplication in automated tests May 18, 2025
@arrestle arrestle force-pushed the AAP-45797-tech-debt branch from ea09312 to 60e9f6f Compare May 18, 2025 13:34
@arrestle arrestle marked this pull request as ready for review May 18, 2025 13:37
@arrestle arrestle requested review from dleehr, lranjbar and AaronH88 May 18, 2025 13:37
Copy link
Collaborator

@dleehr dleehr left a comment

Choose a reason for hiding this comment

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

Thanks for getting this in so early @arrestle.

I've skimmed the refactoring in the _test.go and utils files, and that looks good. Just a couple of points there. I also ran the acceptance tests locally before/after and confirmed the test coverage has not decreased. I would appreciate if others could weigh in as well on the refactored logic.

I'm requesting changes on two specific areas:

  1. There are changes in the README and Makefile around what to run to support the acceptance tests. I think we'll be addressing that in a future ticket around contribution guidelines, so I don't think we're ready for this change and my request is that we hold these back.
  2. There are additions to GetWithStatus that enhance error checking. I had some specific feedback on individual checks as well and some general refactoring feedback. The ask here is to review these checks and see what can be refactored, removed, or changed so that details aren't lost.

Copy link
Contributor

@lranjbar lranjbar left a comment

Choose a reason for hiding this comment

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

Lets collapse the Makefile target into one and use environment variables to set it up.

@arrestle arrestle force-pushed the AAP-45797-tech-debt branch from e031d36 to 183c71b Compare May 19, 2025 19:57
@arrestle arrestle requested a review from tyraziel May 20, 2025 12:04
Copy link
Collaborator

@dleehr dleehr left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. LGTM.

@arrestle arrestle dismissed tyraziel’s stale review May 20, 2025 13:03

conversed on slack.

@arrestle arrestle merged commit e46a5d6 into ansible:main May 20, 2025
4 checks passed
@arrestle arrestle deleted the AAP-45797-tech-debt branch May 20, 2025 13:03
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.

4 participants