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

Ft/paralleltesting #849

Merged
merged 1 commit into from
Aug 11, 2017
Merged

Ft/paralleltesting #849

merged 1 commit into from
Aug 11, 2017

Conversation

nicolas2bert
Copy link
Contributor

@nicolas2bert nicolas2bert commented Aug 3, 2017 •
edited
Loading

No description provided.

@ironman-machine
Copy link
Contributor

PR has been updated. Reviewers, please be cautious.

@ironman-machine
Copy link
Contributor 

CONFLICT (add/add): Merge conflict in tests/unit/api/objectReplicationMD.js
CONFLICT (add/add): Merge conflict in tests/unit/api/objectGet.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/object/get.js
CONFLICT (add/add): Merge conflict in lib/data/wrapper.js
CONFLICT (add/add): Merge conflict in lib/api/objectPutCopyPart.js
CONFLICT (add/add): Merge conflict in lib/api/objectGet.js
CONFLICT (add/add): Merge conflict in lib/api/objectCopy.js
CONFLICT (add/add): Merge conflict in lib/api/completeMultipartUpload.js
CONFLICT (add/add): Merge conflict in lib/api/apiUtils/object/storeObject.js
CONFLICT (add/add): Merge conflict in lib/api/apiUtils/object/createAndStoreObject.js
CONFLICT (add/add): Merge conflict in docker-entrypoint.sh
CONFLICT (add/add): Merge conflict in circle.yml

dora-korpar
dora-korpar reviewed Aug 3, 2017
View reviewed changes
tests.js Outdated
});
}

if (process.env.CIRCLE_NODE_INDEX === '0') {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the CIRCLE_NODE_INDEX set in CircleCi?

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 :)

@nicolas2bert nicolas2bert force-pushed the ft/paralleltesting branch 4 times, most recently from 4179f19 to 21cd52a Compare August 10, 2017 19:21
rahulreddy
rahulreddy approved these changes Aug 10, 2017
View reviewed changes
Copy link
Collaborator

@rahulreddy rahulreddy left a comment

Choose a reason for hiding this comment

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

This looks good! This looks very good! 🥇
Btw can we force a test to fail and see how it looks like in the CI?
Also, I increased the containers in public CI to run the tests, let's see how it goes.

JianqinWang
JianqinWang reviewed Aug 10, 2017
View reviewed changes
tests.bash Outdated

killandsleep 8000

S3BACKEND=mem npm start > $CIRCLE_ARTIFACTS/server_mem_s3curl.txt & bash wait_for_local_port.bash 8000 40 && npm run ft_s3curl
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same test has been run twice?

JianqinWang
JianqinWang reviewed Aug 10, 2017
View reviewed changes
tests.bash Outdated

killandsleep 8000

S3BACKEND=file S3VAULT=mem MPU_TESTING=yes npm start > $CIRCLE_ARTIFACTS/server_file_awssdk.txt & bash wait_for_local_port.bash 8000 40 && npm run ft_awssdk
Copy link
Contributor

Choose a reason for hiding this comment

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

same test ran twice?

JianqinWang
JianqinWang reviewed Aug 10, 2017
View reviewed changes
tests.bash Outdated

# Run S3 with file Backend + KMS Encryption ; run ft_tests

S3BACKEND=file S3VAULT=mem MPU_TESTING=yes npm start > $CIRCLE_ARTIFACTS/server_file_kms_awssdk.txt & bash wait_for_local_port.bash 8000 40
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly missing: '&& ENABLE_KMS_ENCRYPTION=true npm run ft_awssdk'

JianqinWang
JianqinWang reviewed Aug 10, 2017
View reviewed changes
Copy link
Contributor

@JianqinWang JianqinWang left a comment

Choose a reason for hiding this comment

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

I think there are two tests that are ran twice and one test missing, I've added comments at the lines in question!

JianqinWang
JianqinWang approved these changes Aug 11, 2017
View reviewed changes
Copy link
Contributor

@JianqinWang JianqinWang left a comment

Choose a reason for hiding this comment

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

LGTM

jonathan-gramain
jonathan-gramain reviewed Aug 11, 2017
View reviewed changes
tests.bash
sleep 10
}

if [ $CIRCLE_NODE_INDEX -eq 0 ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it specified somewhere that we have 4 parallel runners? If not, may be good to enforce it so that it does not change under our feet and breaks tests (if there's a way)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's manual, its how we do it in Integration as well.

@LaurenSpiegel LaurenSpiegel merged commit e4e9bae into master Aug 11, 2017
@LaurenSpiegel LaurenSpiegel deleted the ft/paralleltesting branch August 11, 2017 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rahulreddy rahulreddy rahulreddy approved these changes

@jonathan-gramain jonathan-gramain jonathan-gramain left review comments

@dora-korpar dora-korpar dora-korpar left review comments

@JianqinWang JianqinWang JianqinWang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nicolas2bert @ironman-machine @rahulreddy @jonathan-gramain @JianqinWang @dora-korpar @LaurenSpiegel