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

test: skip networked tests if no connection #661

Merged
merged 4 commits into from
Dec 18, 2024
Merged

Conversation

bashbunni
Copy link
Member

@bashbunni bashbunni commented Dec 3, 2024

after all that I just deleted the networked test 😂

@bashbunni bashbunni linked an issue Dec 3, 2024 that may be closed by this pull request
@bashbunni
Copy link
Member Author

bashbunni commented Dec 4, 2024

Hey @caarlos0 or @aymanbagabas I may need some help getting CI fixed here. It seems to be detecting github.com/charmbracelet/glow/v2 as the working directory but I'm not sure how to fix that 🤔

@ottok
Copy link

ottok commented Dec 4, 2024

Thanks for looking into this. If you append "(Closes: #660)" in the git commit title or put "Closes: #660" anywhere in the git commit message body, it will automatically link to that issue, and also close that issue if the commit gets merged on branch "master".

@bashbunni bashbunni requested a review from caarlos0 December 4, 2024 16:32
@caarlos0
Copy link
Member

caarlos0 commented Dec 4, 2024

Hey @caarlos0 or @aymanbagabas I may need some help getting CI fixed here. It seems to be detecting github.com/charmbracelet/glow/v2 as the working directory but I'm not sure how to fix that 🤔

I looked into this, but not sure why its failing..

FWIW we have the url tests that are also skipped due to weird network stuff... maybe worth just removing them

glow_test.go Outdated Show resolved Hide resolved
glow_test.go Outdated
Comment on lines 25 to 29
// Check for network issues.
if errors.As(err, &urlErr) {
t.Logf("Error during execution (args: %s):\n%v", v, err)
t.Skip("Test uses network. Are you connected to the Internet?")
} else {
t.Errorf("Error during execution (args: %s): %T %v", v, err, err)
}

if buf.Len() == 0 {
t.Errorf("Output buffer should not be empty (args: %s)", v)
}

Choose a reason for hiding this comment

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

This could be easier to read

Suggested change
// Check for network issues.
if errors.As(err, &urlErr) {
t.Logf("Error during execution (args: %s):\n%v", v, err)
t.Skip("Test uses network. Are you connected to the Internet?")
} else {
t.Errorf("Error during execution (args: %s): %T %v", v, err, err)
}
if buf.Len() == 0 {
t.Errorf("Output buffer should not be empty (args: %s)", v)
}
// Check for network issues.
if errors.As(err, &urlErr) {
t.Logf("Error during execution (args: %s):\n%v", v, err)
t.Skip("Test uses network. Are you connected to the Internet?")
return
}
t.Errorf("Error during execution (args: %s): %T %v", v, err, err)
if buf.Len() == 0 {
t.Errorf("Output buffer should not be empty (args: %s)", v)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I'm just debugging github actions right now, will be undoing these commits once I figure out what's going on 😅

glow_test.go Outdated
@@ -16,13 +18,21 @@ func TestGlowSources(t *testing.T) {

for _, v := range tt {
t.Run(v, func(t *testing.T) {
var urlErr *net.DNSError

Choose a reason for hiding this comment

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

please move the variable definition aside the errors.As

main.go Outdated
Comment on lines 78 to 81
// a GitHub or GitLab URL (even without the protocol):
var dnsErr *net.DNSError
src, err := readmeURL(arg)
if src != nil && err == nil {
// if there's an error, try next methods...
return src, nil
} else if err != nil && errors.As(err, &dnsErr) {
// fail if there's a network error.
return src, err

Choose a reason for hiding this comment

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

The code could be clearer

Suggested change
// a GitHub or GitLab URL (even without the protocol):
var dnsErr *net.DNSError
src, err := readmeURL(arg)
if src != nil && err == nil {
// if there's an error, try next methods...
return src, nil
} else if err != nil && errors.As(err, &dnsErr) {
// fail if there's a network error.
return src, err
// a GitHub or GitLab URL (even without the protocol):
src, err := readmeURL(arg)
if src != nil && err == nil {
// if there's an error, try next methods...
return src, nil
}
var dnsErr *net.DNSError
if errors.As(err, &dnsErr) {
// fail if there's a network error.
return src, err

main.go Outdated Show resolved Hide resolved
main.go Outdated
@@ -77,11 +77,12 @@ func sourceFromArg(arg string) (*source, error) {

// a GitHub or GitLab URL (even without the protocol):
var dnsErr *net.DNSError
var opErr *net.OpError

Choose a reason for hiding this comment

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

if you are testing net.OpError

then you shouldn't test net.DNSError too

Here are some explanations:
https://stackoverflow.com/questions/69568234/how-do-i-work-out-what-type-of-error-my-error-is-in-go
https://haisum.github.io/2021/08/14/2021-golang-http-errors/

maybe url.Error could be enough for you

@bashbunni bashbunni marked this pull request as draft December 5, 2024 00:03
@bashbunni bashbunni force-pushed the skip-networked-tests branch from 573b982 to 3e612da Compare December 5, 2024 00:56
@bashbunni
Copy link
Member Author

bashbunni commented Dec 5, 2024

ci is failing because it's getting a forbidden response from the http.Get request.
am able to reproduce locally with non-existent repos. We don't have any types that we're wrapping around failed Get requests so these networked tests will fail if there's a non-successful call to github.

I vote to just delete TestGlowSources or maybe break it down and test the pieces that don't need a network to run properly

@bashbunni bashbunni marked this pull request as ready for review December 9, 2024 21:08
@bashbunni bashbunni merged commit 7b3bfac into master Dec 18, 2024
22 checks passed
@bashbunni bashbunni deleted the skip-networked-tests branch December 18, 2024 19:10
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.

Make build work offline
4 participants