Skip to content

test: update mock.Server to use httptest.Server#1145

Merged
Manav-Aggarwal merged 2 commits into
mainfrom
tux/da-test-mock-server
Aug 17, 2023
Merged

test: update mock.Server to use httptest.Server#1145
Manav-Aggarwal merged 2 commits into
mainfrom
tux/da-test-mock-server

Conversation

@tuxcanfly

@tuxcanfly tuxcanfly commented Aug 16, 2023

Copy link
Copy Markdown
Collaborator

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

  • 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
Comment thread da/test/da_test.go Outdated

@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.

looks good, left a comment.

@codecov

codecov Bot commented Aug 16, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage has no change and project coverage change: -0.02% ⚠️

Comparison is base (e0cc388) 55.24% compared to head (d76ce4e) 55.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
- Coverage   55.24%   55.23%   -0.02%     
==========================================
  Files          62       62              
  Lines        6708     6708              
==========================================
- Hits         3706     3705       -1     
- Misses       2606     2607       +1     
  Partials      396      396              

see 1 file with indirect coverage changes

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

@MSevey MSevey added this pull request to the merge queue Aug 17, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 17, 2023
@nashqueue nashqueue added this pull request to the merge queue Aug 17, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 17, 2023
@Manav-Aggarwal Manav-Aggarwal force-pushed the tux/da-test-mock-server branch from 0e4305f to d76ce4e Compare August 17, 2023 15:31
@Manav-Aggarwal Manav-Aggarwal added this pull request to the merge queue Aug 17, 2023
Merged via the queue into main with commit 3967fc2 Aug 17, 2023
@Manav-Aggarwal Manav-Aggarwal deleted the tux/da-test-mock-server branch August 17, 2023 15:43
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