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 functional tests to honor MINT_DATA_DIR properly #879

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Nov 17, 2017

getDataReader() now doesn't rely on multiple arguments
for different behavior, instead getDataReader() always
returns a reader depending on the input file name.

This map of file name maps to size which correspond with
MINT_DATA_DIR requirements.

This fixes a whole bunch of bugs in interpretation, repetition
in tests.

@harshavardhana harshavardhana changed the title Fix functional tests once and for all. Fix functional tests to honor MINT_DATA_DIR properly Nov 17, 2017
@harshavardhana harshavardhana requested review from ebozduman and removed request for kannappanr November 17, 2017 00:45
@@ -6244,7 +6255,7 @@ func testPutObjectWithContextV2() {
}
defer c.RemoveBucket(bucketName)
bufSize := thirtyThreeKiB
Copy link
Contributor

Choose a reason for hiding this comment

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

bufSize can also be picked from the map based on filename

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@harshavardhana harshavardhana force-pushed the fix-functional branch 2 times, most recently from 755c181 to ac44e96 Compare November 17, 2017 01:35
Copy link
Contributor

@poornas poornas left a comment

Choose a reason for hiding this comment

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

LGTM

if getDataDir() != "" {
filepath = getDataDir() + "/" + filename
func getFilePath(filename string) (fp string) {
if mintDataDir == "" {
Copy link
Contributor

@ebozduman ebozduman Nov 17, 2017

Choose a reason for hiding this comment

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

Don't we want to return just the file name (filepath here), when mintDataDir is empty, "", i.e when env var MINT_DATA_DIR is not defined?
If true, then having just return filepath.Join(mintDataDir, filename) would be good enough for both cases (when env var is defined and not defined).

Copy link
Member Author

@harshavardhana harshavardhana Nov 17, 2017

Choose a reason for hiding this comment

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

No we are not returning the file name we are only returning if the filepath can be constructed. because this function is meaningless.. outside of mint context

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the function name to avoid confusion.

@@ -617,9 +628,9 @@ func testPutObjectWithMetadata() {
return
}

// Generate data using 2 parts
// Generate data for using 2 parts
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space before 2

@harshavardhana
Copy link
Member Author

@ebozduman please take a look again.

@@ -617,9 +623,10 @@ func testPutObjectWithMetadata() {
return
}

// Generate data using 2 parts
// Generate data for using 2 parts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment can be more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

@harshavardhana
Copy link
Member Author

Removed all unnecessary comments @kannappanr @ebozduman can you review again?

Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

Failing with MINT_DATA_DIR set. Need to verify it is my setup or the code.

objectName := fmt.Sprintf("test-file-%v", rand.Uint32())
args["objectName"] = objectName

offset := length / 2
if _, err := tempfile.Seek(int64(offset), 0); err != nil {
if _, err = tempfile.Seek(int64(offset), 0); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should not do os.Remove at line 3845

@@ -1525,11 +1528,6 @@ func testFPutObject() {
return
}

err = os.Remove(fName + ".gtar")
Copy link
Collaborator

@kannappanr kannappanr Nov 17, 2017

Choose a reason for hiding this comment

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

This file should be removed. This code is needed, otherwise it will leave an extra file in the /mint/data directory

getDataReader() now doesn't rely on multiple arguments
for different behavior, instead getDataReader() always
returns a reader depending on the input file name.

This map of file name maps to size which correspond with
MINT_DATA_DIR requirements.

This fixes a whole bunch of bugs in interpretation, repetition
in tests.
Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

Tested it on my local setup with MINT_DATA_DIR set. LGTM

@deekoder deekoder merged commit 57a8ae8 into minio:master Nov 17, 2017
@harshavardhana harshavardhana deleted the fix-functional branch November 17, 2017 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants