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 direct collection access to future collection item feature test #6151

Merged
merged 3 commits into from Oct 28, 2017

Conversation

BrandonDusseau
Copy link
Contributor

@BrandonDusseau BrandonDusseau commented Jun 18, 2017

Further tests #5953
/cc @jekyll/build

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Is there a reason you didn't create a new tests here?

@BrandonDusseau
Copy link
Contributor Author

@parkr It would have effectively made a duplicate of the test I modified. Since the scenario described matched the situation I was testing, I modified it. I can split it out into another test if you'd like.

@@ -128,16 +128,18 @@ Feature: Collections
puppies:
output: false
"""
And I have a "foo.txt" file that contains "random static file"
And I have a "index.html" page that contains "Newest puppy: {% assign puppy = site.puppies.last %}{{ puppy.title }}"
Copy link
Member

Choose a reason for hiding this comment

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

I really thought site.puppies.first would be the newest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my experience, apparently that is not the case. The documentation doesn't seem to specify.

@BrandonDusseau
Copy link
Contributor Author

Split this into a separate test as previously suggested.

Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

One small tiny change and then I think we're good to go.

@@ -139,6 +139,28 @@ Feature: Collections
And the _site directory should exist
And the "_site/puppies/fido.html" file should not exist

Scenario: Hidden collection with document with future date, accessed via Liquid
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicky, but could you change this to Hidden collection has document with future date, accessed via Liquid

I had trouble parsing the scenario description until I read the full test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem. I was just keeping with the phrasing on the prior test. That'll be fixed in just a sec.

@mattr-
Copy link
Member

mattr- commented Oct 28, 2017

Thanks! Appreciate you adding this test! 👍

@mattr-
Copy link
Member

mattr- commented Oct 28, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 49fa2de into jekyll:master Oct 28, 2017
jekyllbot added a commit that referenced this pull request Oct 28, 2017
@BrandonDusseau BrandonDusseau deleted the future-collection-liquid-test branch October 28, 2017 16:01
@parkr
Copy link
Member

parkr commented Oct 29, 2017

@mattr- I believe this is a failing test showing a behavior that isn’t working right. I see master as currently failing cucumber now on all ruby versions. Would you mind sending a patch to fix master and retry this patch with the proper fix to make the test pass?

@mattr-
Copy link
Member

mattr- commented Oct 29, 2017

Oh! My bad! Why did I think this was passing earlier? 😞

Yup! I'll get right on fixing it.

mattr- added a commit that referenced this pull request Oct 29, 2017
mattr- added a commit that referenced this pull request Oct 29, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants