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

fix(NODE-4998): use ipv4 in socks5 tests for node 18 #3538

Merged
merged 3 commits into from Jan 26, 2023
Merged

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jan 24, 2023

Description

What is changing?

  • Changes the proxy host addresses to be IPv4 to avoid node 18 dns issue
  • Splits socks5 driver only run from socks5 csfle run

What is the motivation for this change?

Green ci

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken marked this pull request as ready for review January 24, 2023 19:35
durran
durran previously approved these changes Jan 25, 2023
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

NODE-4897 addresses the timeout on the release test and the other failure is NODE-4646 flaky test ticket.

@durran durran added the Team Review Needs review from team label Jan 25, 2023
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Currently, the two tasks are

  • socks5 tests only
  • socks5 tests and csfle + socks5 tests

We want to break them apart entirely, so

  • one task for socks5 driver tests
  • one task for csfle tests w/ socks5 enabled

@durran
Copy link
Member

durran commented Jan 25, 2023

Currently, the two tasks are

* socks5 tests only

* socks5 tests and csfle + socks5 tests

We want to break them apart entirely, so

* one task for socks5 driver tests

* one task for csfle tests w/ socks5 enabled

Ah good catch.

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

optional change - we could break apart the auth tests too, and have four total tasks here

  • socks5 no auth
  • socks5 auth
  • csfle socks5 no auth
  • csfle socks5 auth

I'm partial to this because I when evergreen shows the results of the test run (which it doesn't if you have multiple test runs in a task), but this is an optional change

@durran
Copy link
Member

durran commented Jan 25, 2023

optional change - we could break apart the auth tests too, and have four total tasks here

* socks5 no auth

* socks5 auth

* csfle socks5 no auth

* csfle socks5 auth

I'm partial to this because I when evergreen shows the results of the test run (which it doesn't if you have multiple test runs in a task), but this is an optional change

Maybe a new ticket for that effort?

@durran durran merged commit 425dbe0 into main Jan 26, 2023
@durran durran deleted the NODE-4998-fle-socks5 branch January 26, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants