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

added a test comparing the javascript release and the java port #29

Merged
merged 6 commits into from
Dec 7, 2016

Conversation

MartinTeeVarga
Copy link

We are looking for a port of the Zxcvbn library. Yours looks good, but I've created a test that compares the strength of the passwords in the MeasureTest calculated from your Java version with the calculated strength from the the original Javascript and found some differences. It also relates to your issue #8
The test is currently failing for three passwords and the long password has been commented out (the one failing in the original v4.4.1).

@vvatanabe
Copy link
Member

@SM4 Thank you for PR. I will fix it soon. Please wait a few.

@vvatanabe
Copy link
Member

@SM4 I applied the fix code to the master branch.
f31ba6e

@MartinTeeVarga
Copy link
Author

Thanks. I see a null pointer exception when running the JavaPortTest from Travic CI. It's the loading of the resource (the javascript file). I am not seeing the same problem locally. Any ideas?

Also do you want to make this test part of your testing? It's passing locally now with the changes you have made.

@BeforeClass
public static void initEngine() {
ScriptEngineManager manager = new ScriptEngineManager();
engine = manager.getEngineByName("nashorn");
Copy link
Member

Choose a reason for hiding this comment

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

@SM4
The Java environment set in .travis.yml is java 1.7. Using Nashorn requires java 1.8. Try passing string arg "js" to ScriptEngineManager#getEngineByName.

engine = manager.getEngineByName("js");

@MartinTeeVarga
Copy link
Author

I've added a compatibility fix for Rhino (I believe that's the JS engine in JDK 7).

@vvatanabe
Copy link
Member

@SM4 Awesome! Thank you for a wonderful fix!

LGTM

@vvatanabe vvatanabe merged commit b54f099 into nulab:master Dec 7, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.759% when pulling ef8ad41 on sm4:issue-8-javascript-test into f31ba6e on nulab:master.

@MartinTeeVarga MartinTeeVarga deleted the issue-8-javascript-test branch December 7, 2016 06:05
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.

3 participants