Skip to content

Conversation

@gus-costa
Copy link
Contributor

No description provided.

@gus-costa
Copy link
Contributor Author

Hi @sah2ed , thanks for your help. The changes you requested are done.
Let me know if you need something else.

@sah2ed sah2ed merged commit f929900 into topcoder-platform:dev Apr 16, 2017
var testUser = process.env.TEST_USER;
var testPassword = process.env.TEST_PASSWORD;
var testPort = process.env.TEST_PORT || 3000;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Prior to the start of the tests, there should be a check like:

if (testUser == null || testPassword == null) {
// print out warning explaining that tests will fail unless TEST_USER and TEST_PASSWORD environment variables are set.
}

@sah2ed
Copy link
Collaborator

sah2ed commented Apr 16, 2017

@GSTVAC

I ran the build now without setting the env variables and it wasn't easily apparent why the tests failed on CircleCI.

It was clearly stated in Requirement 2.2.1 that "If either variable is unset, the tests should fail hinting that they must be set for the tests to run."

Please see my review remark above on how to remedy the issue: https://github.com/topcoder-platform/admin-app/pull/65/files#diff-f1b7e32bde00048fe7a9882a0fbaa700R6

@gus-costa
Copy link
Contributor Author

@sah2ed
I agree with you. The message gets lost in the middle of the stack traces.
I decided to throw an exception to make the message more clear. Is it good like this or do you prefer to show just a warning?

@gus-costa
Copy link
Contributor Author

I submitted another PR with these changes. #66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants