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

Start adding unit tests #1

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
4 changes: 3 additions & 1 deletion cmd/datasetIngestor/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ import (

const TOTAL_MAXFILES = 400000

// isFlagPassed checks if a specific command-line flag has been passed to the program.
func isFlagPassed(name string) bool {
found := false
// The flag.Visit function is a built-in function from the Go standard library's flag package. It iterates over all command-line flags that have been set (i.e., flags that were provided when the program was run), and for each flag, it calls the provided function.
flag.Visit(func(f *flag.Flag) {
if f.Name == name {
found = true
Expand Down Expand Up @@ -281,7 +283,7 @@ func main() {
} else if numFiles > TOTAL_MAXFILES {
tooLargeDatasets++
color.Set(color.FgRed)
log.Println("This dataset exceeds the current filecount limit of the archive system of %v files and will therefore NOT be stored.\n", TOTAL_MAXFILES)
log.Printf("This dataset exceeds the current filecount limit of the archive system of %v files and will therefore NOT be stored.\n", TOTAL_MAXFILES)
color.Unset()
} else {
datasetIngestor.UpdateMetaData(client, APIServer, user, originalMap, metaDataMap, startTime, endTime, owner, tapecopies)
Expand Down
90 changes: 90 additions & 0 deletions cmd/datasetIngestor/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package main

import (
"bytes"
"os"
"testing"
// "flag"
Copy link
Member

Choose a reason for hiding this comment

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

remove if not needed

)

// TestMainHelp is a test function that verifies the output of the main function.
Copy link
Member

Choose a reason for hiding this comment

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

general comment: thanks a lot for the comments, they are very useful! I think after some time, we can simply discard them, as otherwise the text will become a little too dense, especially if they are on third-party or native go functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, eventually the comments could be reduced.

// It captures the stdout, runs the main function, and checks if the output contains the expected strings.
// This just checks if the main function prints the help message.
func TestMainHelp(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

food for thought: I think this could be simplified mocking fmt.Printf and asserting that this is called with "\n\nTool to ingest datasets to the data catalog.\n\n". https://stackoverflow.com/questions/52315410/mocking-go-methods. This could be some googling though, so happy to keep it as is!

// Capture stdout
// The variable 'old' stores the original value of the standard output (os.Stdout).
old := os.Stdout
// r is a ReadCloser that represents the read end of the pipe.
// w is a WriteCloser that represents the write end of the pipe.
// err is an error variable.
// The os.Pipe() function in Go is used to create a synchronous in-memory pipe. It can be used for communication between different parts of the program.
// The `os.Pipe()` function in Go is used to create a synchronous in-memory pipe. It can be used for communication between different parts of your program.
// This function returns two values: a `*os.File` for reading and a `*os.File` for writing. When you write data to the write end of the pipe, it becomes available to read from the read end of the pipe. This can be useful for passing data between goroutines or between different parts of your program without using the disk.
r, w, err1 := os.Pipe()
if err1 != nil {
// The Fatalf method is similar to log.Fatalf or fmt.Printf in that it formats a string according to a format specifier and arguments, then logs that string as an error message. However, in addition to this, Fatalf also ends the test immediately. No further code in the test function will be executed, and the test will be marked as failed.
t.Fatalf("Could not start the test. Error in reading the file: %v", err1)
}
// redirect the standard output (os.Stdout) to a different destination, represented by w.
// By default, anything written to os.Stdout will be printed to the terminal.
// The w in this line of code is expected to be a value that satisfies the io.Writer interface, which means it has a Write method. This could be a file, a buffer, a network connection, or any other type of destination for output.
// Since w is connected to r, anything written to w can be read from r. This is how we will capture the output of the main function.
os.Stdout = w

// Restore stdout after the test
// defer is a keyword that schedules a function call to be run after the function that contains the defer statement has completed.
defer func() {
Copy link
Member

Choose a reason for hiding this comment

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

I saw golang has setup/tear down test syntax: https://pkg.go.dev/testing#hdr-Main. Maybe this is fine like it is for now though...

os.Stdout = old
}()

// Run main function (assuming your main function does not take any arguments)
main()

// Close pipe writer to flush the output
w.Close()

//declares a variable named buf of type bytes.Buffer. The bytes.Buffer type is a struct provided by the Go standard library that implements the io.Reader and io.Writer interfaces.
var buf bytes.Buffer
// Copy pipe reader output to buf
// ReadFrom reads data from the given reader r and writes it to the buffer buf.
// It returns the number of bytes read and any error encountered.
_, err := buf.ReadFrom(r)
if err != nil {
t.Fatalf("Error reading output: %v", err)
}

// Check if the output contains expected strings
expected := "\n\nTool to ingest datasets to the data catalog.\n\n"
//The bytes.Contains function takes two arguments, both of which are slices of bytes, and checks if the second argument is contained within the first.
// Here, expected is a string, and []byte(expected) converts that string to a slice of bytes.
if !bytes.Contains(buf.Bytes(), []byte(expected)) {
t.Errorf("Expected output %q not found in %q", expected, buf.String())
}
}

// Errors with the test above.
Copy link
Member

Choose a reason for hiding this comment

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

what's this part? I'd rather remove it than have it commented, we can add it later.

// // TestIsFlagPassed is a test function that verifies the output of the isFlagPassed function. It checks whether the function returns the expected results for defined and undefined flags.
// func TestIsFlagPassed(t *testing.T) {
// // Save original os.Args
// oldArgs := os.Args
// defer func() { os.Args = oldArgs }()

// // Define a new flag for testing
// flag.String("testenv", "", "testenv flag")

// // Set os.Args to simulate command-line input
// os.Args = []string{"cmd", "-testenv", "false"}

// // Parse flags
// flag.Parse()

// // Test with a flag that was defined
// if !isFlagPassed("testenv") {
// t.Errorf("isFlagPassed() failed for flag that was defined")
// }

// // Test with a flag that was not defined
// if isFlagPassed("undefinedflag") {
// t.Errorf("isFlagPassed() succeeded for flag that was not defined")
// }
// }
Copy link
Member

Choose a reason for hiding this comment

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

missing newline. Maybe consider adding some linting in the CI, so this will fail the tests. https://golangci-lint.run/ (as a side comment, I think the CI addition should have been a PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think such things would fail the tests. It didn't fail here because it was a comment.

8 changes: 4 additions & 4 deletions cmd/datasetIngestor/metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
"creationLocation": "/PSI/SLS/CSAXS",
"datasetName": "CMakeCache",
"description": "",
"owner": "Ana Diaz",
"ownerEmail": "ana.diaz@psi.ch",
"ownerGroup": "p17301",
"principalInvestigator": "ana.diaz@psi.ch",
"owner": "Ali Vahdati",
"ownerEmail": "reza.rezaee-vahdati@psi.ch",
"ownerGroup": "[email protected]",
"principalInvestigator": "reza.rezaee-vahdati@psi.ch",
"scientificMetadata": [
{
"sample": {
Expand Down
26 changes: 13 additions & 13 deletions cmd/datasetPublishData/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,20 @@ on the publication server
package main

import (
"bufio"
"bytes"
"crypto/tls"
"encoding/json"
"flag"
"fmt"
"html/template"
"io/ioutil"
"log"
"net/http"
"os"
"os/exec"
"bufio" // Provides buffered I/O operations for efficient reading and writing
"bytes" // Provides functions for operating on byte slices
"crypto/tls" // Provides functionality for secure TLS/SSL communication
"encoding/json" // Encodes and decodes JSON data
"flag" // Parses command-line flags
"fmt" // Provides formatted I/O operations
"html/template" // Parses and executes HTML templates
"io/ioutil" // Reads and writes files
"log" // Writes messages to a log file
"net/http" // Provides network communication for HTTP requests
"os" // Provides access to operating system functionality
"os/exec" // Executes external commands
"scicat/datasetUtils"
"strings"
"strings" // Provides functions for manipulating strings
"time"
"unicode/utf8"

Expand Down
4 changes: 2 additions & 2 deletions cmd/datasetRetriever/main.go
Copy link
Member

Choose a reason for hiding this comment

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

I think all these changes shouldn't be here, as they are not covered by tests and we should be able to revert if they fail. So they should be in another PR, potentially with dedicated tests when you'll tackle this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes cannot be removed or the tests fail. although they are not directly checked for these tests, they are simply wrong code that causes the tests to fail.

Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func main() {
log.Printf("You are about to retrieve dataset(s) from the === %s === retrieve server...", env)
color.Unset()

if *retrieveFlag == false {
if !*retrieveFlag {
color.Set(color.FgRed)
log.Printf("Note: you run in 'dry' mode to simply check which data would be fetched.\n")
log.Printf("Use the -retrieve flag to actually transfer the datasets to your chosen destination path.\n")
Expand Down Expand Up @@ -126,7 +126,7 @@ func main() {

if len(datasetList) == 0 {
fmt.Printf("\n\nNo datasets found on intermediate cache server.\n")
fmt.Println("Did you submit a retrieve job from the data catalog first ?\n")
fmt.Println("Did you submit a retrieve job from the data catalog first ?")
} else {
// get sourceFolder and other dataset related info for all Datasets
datasetDetails := datasetUtils.GetDatasetDetails(client, APIServer, user["accessToken"], datasetList, *ownerGroup)
Expand Down
2 changes: 1 addition & 1 deletion datasetIngestor/assembleFilelisting.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func AssembleFilelisting(sourceFolder string, filelistingPath string, skip *stri
// spin.Start() // Start the spinner
e := filepath.Walk(line, func(path string, f os.FileInfo, err error) error {
// ignore ./ (but keep other dot files)
if f == nil || f.IsDir == nil || f.Name == nil {
if f == nil || !f.IsDir() || f.Name() == "" {
log.Printf("Missing file info for line %s and path %s", line, path)
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion datasetUtils/getUserInfoFromToken.go
Copy link
Member

Choose a reason for hiding this comment

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

I think all these changes shouldn't be here, as they are not covered by tests and we should be able to revert if they fail. So they should be in another PR, potentially with dedicated tests when you'll tackle this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func GetUserInfoFromToken(client *http.Client, APIServer string, token string) (
accessGroups = respObj.CurrentGroups
log.Printf("User is member in following groups: %v\n", accessGroups)
} else {
log.Fatalln("Could not map a user to the token %v", token)
log.Fatalf("Could not map a user to the token %v", token)
}
return u, accessGroups

Expand Down
Loading