Skip to content

test: use pkg httptest to avoid port conflict#1144

Merged
nashqueue merged 2 commits into
mainfrom
tux/fix-da-test-port
Aug 16, 2023
Merged

test: use pkg httptest to avoid port conflict#1144
nashqueue merged 2 commits into
mainfrom
tux/fix-da-test-port

Conversation

@tuxcanfly

@tuxcanfly tuxcanfly commented Aug 16, 2023

Copy link
Copy Markdown
Collaborator

Overview

This PR uses pkg httptest to avoid port conflict when running tests under da/test i.e. TestLifecycle, TestRetrieve.

This allows the tests to pass even if an address is bound to the default port i.e. 127.0.0.1:26658

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@tuxcanfly tuxcanfly added the T:testing Related to testing label Aug 16, 2023
@tuxcanfly tuxcanfly changed the title test: use pkg httptest to avoid port allocation test: use pkg httptest to avoid port conflict Aug 16, 2023
@codecov

codecov Bot commented Aug 16, 2023

Copy link
Copy Markdown

Codecov Report

Patch and project coverage have no change.

Comparison is base (76dd754) 55.14% compared to head (2eebd31) 55.14%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1144   +/-   ##
=======================================
  Coverage   55.14%   55.14%           
=======================================
  Files          62       62           
  Lines        6719     6719           
=======================================
  Hits         3705     3705           
  Misses       2618     2618           
  Partials      396      396           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Manav-Aggarwal Manav-Aggarwal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@nashqueue nashqueue added this pull request to the merge queue Aug 16, 2023
Merged via the queue into main with commit 91e1c64 Aug 16, 2023
@nashqueue nashqueue deleted the tux/fix-da-test-port branch August 16, 2023 18:03
github-merge-queue Bot pushed a commit that referenced this pull request Aug 17, 2023
## Overview

This PR improves upon #1144 by simplifying `mock.Server` to use
`httptest` directly, avoiding a duplicate `http.Server`. `Server.Start`
doesn't need a listener anymore, and it returns the URL at which the
`httptest.Server` is running.

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords
Manav-Aggarwal pushed a commit that referenced this pull request Sep 6, 2023
## Overview

This PR uses pkg `httptest` to avoid port conflict when running tests
under `da/test` i.e. `TestLifecycle`, `TestRetrieve`.

This allows the tests to pass even if an address is bound to the default
port i.e. `127.0.0.1:26658`

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords
Manav-Aggarwal pushed a commit that referenced this pull request Sep 6, 2023
## Overview

This PR improves upon #1144 by simplifying `mock.Server` to use
`httptest` directly, avoiding a duplicate `http.Server`. `Server.Start`
doesn't need a listener anymore, and it returns the URL at which the
`httptest.Server` is running.

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T:testing Related to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants