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

Add a few tests for an issue under open investigation #1513

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stevenaw
Copy link
Member

@stevenaw stevenaw commented Nov 9, 2024

Relates to #1510

@CharliePoole
Copy link
Collaborator

@stevenaw I'm back from my class and looking this PR over. Do I understand correctly that the first commit is what should work, while the second simply changes the tests to reflect current behavior?

@stevenaw
Copy link
Member Author

Welcome back @CharliePoole

Admittedly, my investigation here was partly self-exploration. The first commit was to try what had been attempted in the original reporter's repro, and then how the tokenizer interpreted it. The second commit updates it to be enclosed in commas after I noticed other places where you had recommended the same. This, of course, changed the tokenizer output and so I updated the test accordingly.

My own lack of knowledge about this part of the codebase prevents me from saying which of these two is correct, though the fact that the engine could parse the comma-enclosed value but the framework couldn't resolve it to a test had me was beginning to wonder if it were a framework issue.

I'd welcome your investigation here if you are able. If you did, please do feel free to close this PR or to build upon it depending on the direction it may go.

@CharliePoole
Copy link
Collaborator

Thanks, that gives me the context I wanted. I'll adopt this branch and see what I can figure out. It really seems that we need tests to verify the XML content of each example as well as tests that exercise the framework using the same filters.

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.

2 participants