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

Improve tests for Jekyll::PluginManager #5167

Merged
merged 4 commits into from Aug 7, 2016

Conversation

Projects
None yet
3 participants
@Crunch09
Member

Crunch09 commented Jul 30, 2016

Hi,
this tries to improve the tests for the Jekyll::PluginManager by doing two things:

  • migrate the existing tests to should syntax (those were the last in the test dir with def test_* syntax
  • add missing tests

When running script/test test/test_plugin_manager.rb:

Before
screen shot 2016-07-30 at 11 22 31

After
screen shot 2016-07-30 at 15 16 21

Crunch09 added some commits Jul 30, 2016

Show outdated Hide outdated test/test_plugin_manager.rb
context "JEKYLL_NO_BUNDLER_REQUIRE set to `true`" do
should "not require from bundler" do
with_env("JEKYLL_NO_BUNDLER_REQUIRE", "true") do
assert_equal false, Jekyll::PluginManager.require_from_bundler,

This comment has been minimized.

@parkr

parkr Aug 6, 2016

Member

I believe you can use refute here, which is the same as asserting false.

@parkr

parkr Aug 6, 2016

Member

I believe you can use refute here, which is the same as asserting false.

Show outdated Hide outdated test/test_plugin_manager.rb
plugin_manager = PluginManager.new(site)
expect(plugin_manager).to receive(:plugin_allowed?).with("jemoji")
expect(plugin_manager).to receive(:plugin_allowed?).with("foobar")

This comment has been minimized.

@parkr

parkr Aug 6, 2016

Member

Why does this assert that the plugins were actually required?

@parkr

parkr Aug 6, 2016

Member

Why does this assert that the plugins were actually required?

This comment has been minimized.

@Crunch09

Crunch09 Aug 7, 2016

Member

Good point! I'll update this test to proof they are required instead of allowed.

@Crunch09

Crunch09 Aug 7, 2016

Member

Good point! I'll update this test to proof they are required instead of allowed.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 6, 2016

Member

This is wonderful! Thank you! Two general comments above, otherwise 👍

Member

parkr commented Aug 6, 2016

This is wonderful! Thank you! Two general comments above, otherwise 👍

Crunch09 added some commits Aug 7, 2016

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Aug 7, 2016

Member

@parkr Thanks for the review! I tried to address your comments in the added commits

Member

Crunch09 commented Aug 7, 2016

@parkr Thanks for the review! I tried to address your comments in the added commits

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 7, 2016

Member

LGTM. Thank you! 👍

@jekyllbot: merge +dev

Member

parkr commented Aug 7, 2016

LGTM. Thank you! 👍

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 554749a into jekyll:master Aug 7, 2016

1 of 3 checks passed

jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Aug 7, 2016

@Crunch09 Crunch09 deleted the Crunch09:plugin_manager_tests branch Aug 7, 2016

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