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.

Sign up for GitHub

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

Jump to bottom

fix: set default port to 3020 #37

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: set default port to 3020 #37

wants to merge 5 commits into from

Conversation

krushnarout
Copy link

@krushnarout krushnarout commented Jan 23, 2025 •
edited
Loading

/claim #36
fixes #36

Screen.Recording.2025-01-23.at.11.15.06.AM.2.mov

@algora-pbc algora-pbc bot mentioned this pull request Jan 23, 2025
Open
@krushnarout
Copy link
Author

@seveibar I've added the test file, and the tests are passing.

Screenshot 2025-01-23 at 11 56 46 AM

@seveibar seveibar mentioned this pull request Jan 23, 2025
Closed
seveibar
seveibar requested changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@seveibar seveibar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

don't start two dev servers in the test, also improve the readability. The test should be much, much more obvious, it probably has 2x as many lines as it needs

(similar feedback as the other PR, you can put a service on 3020 without starting two dev servers)

@krushnarout krushnarout requested a review from seveibar January 23, 2025 07:10
seveibar
seveibar requested changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

The test should bind something to 3020, then check that the dev server goes on 3021 using a health check. Don't repeat the function isPortAvailable, it's not needed in the test

reviewed on stream ~0:08:00

@krushnarout krushnarout requested a review from seveibar January 23, 2025 18:06
@krushnarout
Copy link
Author

The test should bind something to 3020, then check that the dev server goes on 3021 using a health check. Don't repeat the function isPortAvailable, it's not needed in the test

reviewed on stream ~0:08:00

Done! Please check and let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seveibar seveibar Awaiting requested review from seveibar

Assignees
No one assigned
Labels
🙋 Bounty claim
Projects
None yet
Milestone
No milestone
2 participants
@krushnarout @seveibar