-
Notifications
You must be signed in to change notification settings - Fork 490
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
#4438 add HarvestingServerIT now that 4096 is merged #4577
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if http://localhost:8083 were configurable.
conf/docker-aio/run-test-suite.sh
Outdated
@@ -1,4 +1,4 @@ | |||
#!/bin/sh | |||
# This is the canonical list of which "IT" tests are expected to pass. | |||
# Please note the "dataverse.test.baseurl" is set to run for "all-in-one" Docker environment. | |||
mvn test -Dtest=DataversesIT,DatasetsIT,SwordIT,AdminIT,BuiltinUsersIT,UsersIT,UtilIT,ConfirmEmailIT,FileMetadataIT,FilesIT,SearchIT,InReviewWorkflowIT -Ddataverse.test.baseurl='http://localhost:8083' | |||
mvn test -Dtest=DataversesIT,DatasetsIT,SwordIT,AdminIT,BuiltinUsersIT,UsersIT,UtilIT,ConfirmEmailIT,FileMetadataIT,FilesIT,SearchIT,InReviewWorkflowIT,HarvestingServerIT -Ddataverse.test.baseurl='http://localhost:8083' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pameyer how do you feel about making http://localhost:8083
configurable? Perhaps a different URL could be passed in as an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it configurable seems reasonable. How about checking for an environmental variable ($DV_HOST
), and using a default if it's not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a slight preference for a command line argument or flag over an environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pameyer how do you feel about the getopts stuff in http://guides.dataverse.org/en/4.8.6/developers/testing.html#download-files-sh-script ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me barring the discussed change to add a variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks @pameyer !
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
One line change to update the list of expected to pass integration tests in docker-aio
Pull Request Checklist