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.

Sign up for GitHub

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

Jump to bottom

Missing test coverage from pull request #1511? #1806

Open
jayaddison opened this issue Feb 18, 2023 · 5 comments · May be fixed by #1807
Open

Missing test coverage from pull request #1511? #1806

jayaddison opened this issue Feb 18, 2023 · 5 comments · May be fixed by #1807

Comments

@jayaddison
Copy link

As part of pull request #1511, the changes may have fixed a bug related to native type rendering for iterator; it could be worth adding test coverage related to that.

Before #1511, the code was calling itertools.islice(values, 2) to retrieve the head elements from a values collection (potentially of an iterator type) and then using itertools.chain(head, values) to subsequently produce output.

In the case where values is an iterator, I think that works correctly. But in cases where values is not an iterator, it can result in output duplication (something that I think #1511 indirectly resolves with this check).

For example:

>>> from itertools import islice
>>> values = [1,2,3,4]
>>> head = islice(values, 2)
>>> from itertools import chain
>>> list(chain(head, values))
[1, 2, 1, 2, 3, 4]
@jayaddison
Copy link
Author

(note: opened this as a separate issue thread since the existing thread(s) have been locked)

@davidism
Copy link
Member

Happy to consider a PR that fixes this or adds tests.

@jayaddison
Copy link
Author

jayaddison commented Feb 18, 2023 •
edited

Work-in-progress at https://github.com/openculinary/jinja/tree/issue-1806/pr-1511-additional-test-coverage:

  • Test coverage to demonstrate that a problem could have occurred during async rendering of templates containing native types
  • Enable async support for pytest and tox (doesn't appear to be required; continuous integration already includes async unit test coverage)

@jayaddison jayaddison linked a pull request Feb 19, 2023 that will close this issue
Open
6 tasks
@jayaddison
Copy link
Author

Does the test coverage offered in #1807 seem reasonable for this? (that's one of my pull requests, and I'm trying to close/refresh a few that have been open for a while)

@davidism
Copy link
Member

Seems fine, just haven't had time to get to merges here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Additional test coverage for async rendering of native type templates openculinary/jinja
2 participants
@davidism @jayaddison