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

A few more unit tests #73

Closed
wants to merge 6 commits into from
Closed

A few more unit tests #73

wants to merge 6 commits into from

Conversation

lightglitch
Copy link
Contributor

Don't know what is the best option to test the private methods of DefaultExtensionFinder ...

@decebals
Copy link
Member

I am so happy when I see a new unit test 😄 I try to learn as much as possible from this project. I am sure that we must modified (a little bit) the project code to facilitate the creation of unit tests.
Also I think that we must use (as much as possible) immutable (not changeable) classes with Builder pattern for instantiation to facilitate the thread safe implementation.
I will come with comments at this PR asap. Thanks!

@lightglitch
Copy link
Contributor Author

My experience in unit testing is also not very high so any suggestions are welcome.

@@ -51,10 +51,10 @@ public DefaultExtensionFinder(PluginManager pluginManager, ExtensionFactory exte
}

log.debug("Finding extensions for extension point '{}'", type.getName());
readIndexFiles();
Map<String, Set<String>> cachedEntries = readIndexFiles();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this modification. What was wrong in the previous version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was to make it easy to test but if you have another suggestion I can revert this.

Copy link
Member

Choose a reason for hiding this comment

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

Can you do a rebase please. I modified the class a little bit to improve the testability.

@decebals
Copy link
Member

Don't know what is the best option to test the private methods of DefaultExtensionFinder ...

Maybe this link help.

@janhoy
Copy link
Member

janhoy commented Feb 24, 2017

Just browsing this project to consider it for use. Why is this and two other PR's still open since 2015? I would expect them to be either merged or closed?

@decebals
Copy link
Member

@janhoy
Maybe, because the code is not ready for merge (are some comments without response).
For example I see in one comment:

Can you do a rebase please. I modified the class a little bit to improve the testability.
These PR are not closed because they comes with a big value (improve testability) and I hope to merge them in the future.

Right now for me it's important to merge #128, because this PR comes with useful things and it contains some refactored code. After I release the next version (in few days), I will try to take a look over these old PR/issues and to come with a resolution.

@janhoy
Copy link
Member

janhoy commented Feb 24, 2017

+1

@lightglitch
Copy link
Contributor Author

I have been very busy but I will try to update the pull request soon.

@coveralls
Copy link

coveralls commented Feb 26, 2017

Coverage Status

Coverage remained the same at 13.403% when pulling 5a97a1e on lightglitch:unit-tests into c82df37 on decebals:master.

@@ -0,0 +1,152 @@
/*
* Copyright 2015 Mario Franco.
Copy link
Member

Choose a reason for hiding this comment

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

Please change the copyright (they are some files in this PR).

@lightglitch
Copy link
Contributor Author

The tests need to be updated I will tell you when it's OK to merge it

@decebals
Copy link
Member

@lightglitch I was the impression that you made some changes 😄

@decebals
Copy link
Member

decebals commented Apr 3, 2017

@lightglitch With your permission, I will integrate these tests in master.

@decebals
Copy link
Member

decebals commented Apr 6, 2017

Resolved by 0a0513b.
@lightglitch Thanks!

@decebals decebals closed this Apr 6, 2017
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.

None yet

4 participants