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

Fixed failing tests due to the change we check for file readability. #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixed failing tests due to the change we check for file readability. #9

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa05913
Fixed failing tests due to the change we check for file readability.
kberov Sep 29, 2024
7299137
chg: use sentinel errors
HeikoSchlittermann Sep 29, 2024
5ff7fe4
Used errors.Is() instead of parsing error strings.
kberov Sep 30, 2024
473782c
Fixes suggested by reviewer.
kberov Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func Example_New_err() {
fmt.Println(err.Error())
}
// Output:
// Gledki root directory '/ala/bala' does not exist!
// Gledki root directory '/ala/bala': file does not exist!
}

func ExampleGledki_Execute_simple() {
Expand Down
4 changes: 2 additions & 2 deletions gledki.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,15 +330,15 @@ func (t *Gledki) findRoots(roots []string) error {
t.Roots = append(t.Roots, byCwd)
continue
} else {
return fmt.Errorf("gledki root directory '%s' does not exist! You have to create it. ", byCwd)
return fmt.Errorf("Gledki root directory '%s': %w! You have to create it.", byCwd, os.ErrNotExist)
}
}

if dirExists(root) {
t.Roots = append(t.Roots, root)
continue
} else {
return fmt.Errorf("Gledki root directory '%s' does not exist!", root)
return fmt.Errorf("Gledki root directory '%s': %w!", root, os.ErrNotExist)
}
}
return nil
Expand Down
41 changes: 21 additions & 20 deletions gledki_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ package gledki

import (
"bytes"
"errors"
"fmt"
"io"
"io/fs"
"os"
"path/filepath"
"strings"
"syscall"
"testing"

"github.com/labstack/gommon/log"
Expand Down Expand Up @@ -293,11 +295,10 @@ func TestFtExecString(t *testing.T) {
func TestErrors(t *testing.T) {

if _, err := New([]string{"/ala/bala/nica"}, filesExt, tagsPair, false); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "does not exist") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, os.ErrNotExist) {
t.Logf("Right error: %v", err)
} else {
t.Fatalf("Wrong error: errstr")
t.Fatalf("Wrong error: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly suggest not to use t.Fatal*, unless you really want to abort the test. Better to use t.Error*.

  • t.Error* -> t.Log + t.Fail, which marks the testcase as failed, but continues
  • t.Fatal* -> t.Log + t.FailNow, which aborts the current test function

Same applies to the following test cases

}
} else {
t.Fatal("No error - this is unexpected!")
Expand All @@ -307,10 +308,10 @@ func TestErrors(t *testing.T) {
out.Reset()
if _, err := tpls.Execute(&out, "no_wrapper"); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "could not be read") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, syscall.ENOENT) {
t.Logf("Right error: %s", errstr)
} else {
t.Fatalf("Wrong error: errstr")
t.Fatalf(`Wrong error: %s`, errstr)
}
} else {
t.Fatal("No error - this is unexpected!")
Expand All @@ -319,10 +320,10 @@ func TestErrors(t *testing.T) {
out.Reset()
if _, err := tpls.Execute(&out, "nosuchfile"); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "could not be read") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, syscall.ENOENT) {
t.Logf("Right error: %s", errstr)
} else {
t.Fatalf("Wrong error: errstr")
t.Fatalf("Wrong error: %s", errstr)
}
} else {
t.Fatal("No error - this is unexpected!")
Expand All @@ -331,21 +332,21 @@ func TestErrors(t *testing.T) {
out.Reset()
if _, err := tpls.Execute(&out, "no_include"); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "could not be read") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, syscall.ENOENT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if syscall.ENOENT isn't a way to specific here, the more general os.ErrNotExist should be better suitable, IMHO. I'm not sure about portability of ENOENT (maybe other systems report an other errno, which hopefully will be translated by the Go libraries into os.ErrNotExist

Copy link
Owner Author

Choose a reason for hiding this comment

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

The system returns "no such file or directory". I found that this string comes from syscall.ENOENT and just used it as a sentinel error. os.ErrNotExist contains "file does not exist", which is a different string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The syscall package is os dependend.

GOOS=windows go doc syscall
GOOS=linux go doc syscall

e.g. shows different things. Fortunately for the current situation, syscall.ENOENT seems to be available in both cases. But do we know about other OS?

I'd suggest, using the highest abstraction available, which is os.ErrNotExist, IMHO

t.Logf("Right error: %s", errstr)
} else {
t.Fatalf("Wrong error:%s", errstr)
t.Fatalf("Wrong error: %s", errstr)
}
} else {
t.Fatalf("No error - this is unexpected! Output: %s", out.String())
}
out.Reset()
if _, err := tpls.Execute(&out, "incl_no_wrapper.htm"); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "could not be read") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, syscall.ENOENT) {
t.Logf("Right error: %s", errstr)
} else {
t.Fatalf("Wrong error:%s", errstr)
t.Fatalf("Wrong error: %s", errstr)
}
} else {
t.Fatalf("No error - this is unexpected! Output: %s", out.String())
Expand All @@ -354,8 +355,8 @@ func TestErrors(t *testing.T) {
out.Reset()
if _, err := tpls.Execute(&out, "incl_no_include.htm"); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "could not be read") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, syscall.ENOENT) {
t.Logf("Right error: %s", errstr)
} else {
t.Fatalf("Wrong error:%s", errstr)
}
Expand All @@ -376,8 +377,8 @@ func TestErrors(t *testing.T) {

if err = tpls.findRoots([]string{"../ala/bala"}); err != nil {
errstr := err.Error()
if strings.Contains(errstr, "does not exist!") {
t.Logf("Right error: %s", err.Error())
if errors.Is(err, os.ErrNotExist) {
t.Logf("Right error: %s", errstr)
} else {
t.Fatalf("Wrong error:%s", errstr)
}
Expand Down
Loading