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

Prevent loss of characters in case of false end-partial-match #454

Merged
merged 2 commits into from
Oct 10, 2021

Conversation

timmorey
Copy link
Contributor

@timmorey timmorey commented Oct 4, 2021

Attempted fix for #449

I believe I ran into the same issue, and built on the debugging that @vlascik had done. I don't have a deep understanding of how this code fits into the overall build pipeline, but was able to wrap my head around the code in this module enough to propose a fix.

This addresses the case where we found a partial match for MARKER at the end of the last chunk, but now that we're looking in the next chunk we've realized it wasn't really a match. The fix is to make sure the characters we sliced off of the last chunk don't get lost.

@ef4
Copy link
Collaborator

ef4 commented Oct 4, 2021

Thanks! Nice work.

The tests for this area are here: https://github.com/ef4/ember-auto-import/blob/a88004f656df9e08d3d5e7e3eb337e0cd5727b9e/packages/ember-auto-import/ts/tests/analyzer-test.ts#L379-L490

If you can get a test case in here that fails without your fix and passes with it, we'll be good to go.

@timmorey
Copy link
Contributor Author

timmorey commented Oct 4, 2021

@ef4 thanks for the pointer. Test added.

Comment on lines +493 to +496
assert.ok(
meta.slice(MARKER.length, -MARKER.length).indexOf(MARKER[0]) > -1,
'serialized sample data must contain first character of MARKER somewhere between boundary markers for test to have meaning'
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assertion isn't really checking the result of running code -- its just checking that our sample data is sufficient for this test case. If MARKER or the sample data were changed in the future, such that the first character of MARKER didn't show up somewhere in the serialized data, this test would become invalid.

@ef4 ef4 merged commit 450d321 into embroider-build:main Oct 10, 2021
@ef4
Copy link
Collaborator

ef4 commented Oct 10, 2021

Thanks, this is going out today as 2.2.1.

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