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

WarFile.java:147-148: Implement validation of JSON inside... #16 #18

Merged
merged 5 commits into from
Feb 10, 2015
Merged

WarFile.java:147-148: Implement validation of JSON inside... #16 #18

merged 5 commits into from
Feb 10, 2015

Conversation

nhekfqn
Copy link
Contributor

@nhekfqn nhekfqn commented Feb 8, 2015

No description provided.

@dmarkov
Copy link

dmarkov commented Feb 9, 2015

Let me find a reviewer for this pull request, thanks for submitting it

@dmarkov
Copy link

dmarkov commented Feb 9, 2015

@krzyk please review

@@ -100,4 +103,32 @@ public void checkEbextensionsValidityThrowsExceptionNoConfigFiles()
);
}
}

/**
* Verifies that checkEbextensionsValidity runs fine
Copy link

Choose a reason for hiding this comment

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

  • WarFile can use a .ebextensions with valid json

@krzyk
Copy link

krzyk commented Feb 9, 2015

@nhekfqn few comments above, and you should add a test for invalid json also

@nhekfqn
Copy link
Contributor Author

nhekfqn commented Feb 9, 2015

@krzyk please review.

* @return ZipFile mock.
* @throws IOException Thrown in case of error.
*/
private ZipFile zipWithEbextensionsWithText(final String text)
Copy link

Choose a reason for hiding this comment

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

just zipWithEbextensions(final String text) is enough

@krzyk
Copy link

krzyk commented Feb 9, 2015

@nhekfqn we are almost there :), see my comments above

@nhekfqn
Copy link
Contributor Author

nhekfqn commented Feb 9, 2015

@krzyk

you could just add @test(expected = MojoFailureException.class)

I know, but I couldn't check exception message in that case. MojoFailureException can be thrown from three places in WarFile.checkEbextensionsValidity so its message does matter.

Other defects fixed, please review.

@krzyk
Copy link

krzyk commented Feb 10, 2015

@nhekfqn You can use ExpectedException for checking message

@nhekfqn
Copy link
Contributor Author

nhekfqn commented Feb 10, 2015

@krzyk please review.


/**
* WarFile throws exception when .ebextensions contains invalid json.
* @todo #16 enable test when WarFile.validYaml implemented
Copy link

Choose a reason for hiding this comment

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

@todo #16:15min Enable this test when WarFile.validYaml is implemented.

@krzyk
Copy link

krzyk commented Feb 10, 2015

@nhekfqn a comment about todo formatting

@nhekfqn
Copy link
Contributor Author

nhekfqn commented Feb 10, 2015

@krzyk Please review.

@krzyk
Copy link

krzyk commented Feb 10, 2015

@rultor merge pls

@rultor
Copy link
Contributor

rultor commented Feb 10, 2015

@rultor merge pls

@krzyk OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 0698efe into jcabi:master Feb 10, 2015
@rultor
Copy link
Contributor

rultor commented Feb 10, 2015

@rultor merge pls

@krzyk Done! FYI, the full log is here (took me 8min)

@dmarkov
Copy link

dmarkov commented Feb 16, 2015

@krzyk done, I added 17 mins in payment 51683416... extra minutes for review comments (c=2)... added +17 to your rating, now it is equal to +4210

@dmarkov
Copy link

dmarkov commented Feb 16, 2015

@rultor deploy

@rultor
Copy link
Contributor

rultor commented Feb 16, 2015

@rultor deploy

@dmarkov OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Contributor

rultor commented Feb 16, 2015

@rultor deploy

@dmarkov Done! FYI, the full log is here (took me 6min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants