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

fix(file source): correct an error with line aggregation in continue_through and halt_before #3262

Merged
merged 10 commits into from Jul 31, 2020
Merged

fix(file source): correct an error with line aggregation in continue_through and halt_before #3262

merged 10 commits into from Jul 31, 2020

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Jul 30, 2020

Closes #3237.

@MOZGIII
Hint which data is where
b4641b5 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Add real life test case
d4021f4 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Fix the issue with emitting multiple lines
2c9baa6 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Rename real_life_use_case_1 into two_lines_emit_with_continue_through
459713f 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Adjust the samples at two_lines_emit_with_continue_through for better…
1d4756e 
… formatting

Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Add a two_lines_emit_with_halt_before test
825c97f 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Fix an issue with emitting two messages at HaltBefore mode
3526887 
Signed-off-by: MOZGIII <mike-n@narod.ru>
bruceg
bruceg requested changes Jul 30, 2020
View reviewed changes
Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

The logic of the change looks good, would just like to see a little more comprehensive tests.

src/sources/file/line_agg.rs Outdated Show resolved Hide resolved
src/sources/file/line_agg.rs Outdated Show resolved Hide resolved
src/sources/file/line_agg.rs Outdated Show resolved Hide resolved
@MOZGIII
Add a comment on debug_assert
61ebd38 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII MOZGIII mentioned this pull request Jul 30, 2020
Closed
@MOZGIII
Use pretty_assertions
83b8e9f 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Fix another set of bugs and corrected two_lines_emit_with_halt_before…
22386ec 
… test

- two_lines_emit_with_halt_before had hard to grasp example, which
  resulted in incorrect semantics, and effective a *wrong test*; the
  test samples were rewritten to ensure
- added more sample for the stashing logic tests, and hinted which test
  lines are going to be stashed for easier correspondence of the test logic
  with the code

Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 31, 2020

I gave the tests another look before the merge, and found a set of serious issues. 😅
They're now fixed, and tests are adapted accordingly. @bruceg, could you please take another look?
The relevant commit is 22386ec.

@MOZGIII MOZGIII requested a review from bruceg July 31, 2020 02:20
bruceg
bruceg reviewed Jul 31, 2020
View reviewed changes
src/sources/file/line_agg.rs
@@ -293,6 +293,7 @@ fn add_next_line(buffered: &mut BytesMut, line: Bytes) {
#[cfg(test)]
mod tests {
use super::*;
use pretty_assertions::assert_eq;
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I'll have to remember that one for my own tests. Too badd we can't turn that on across all the sources at once.

Copy link
Contributor Author

@MOZGIII MOZGIII Jul 31, 2020 •
edited

Choose a reason for hiding this comment

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

I noticed it being used at the file/mod.rs and it looked really good.

Can't we import it at /lib.rs with an old #[macro_use] extern crate pretty_assertions;?

@MOZGIII MOZGIII merged commit 0d497ec into master Jul 31, 2020
@MOZGIII MOZGIII deleted the issue-3237 branch July 31, 2020 18:44
@MOZGIII MOZGIII mentioned this pull request Oct 2, 2020
Closed
@MOZGIII MOZGIII mentioned this pull request Oct 14, 2020
Closed
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
@MOZGIII
fix(file source): correct an error with line aggregation in `continue…
ba1c8bf 
…_through` and `halt_before` (vectordotdev#3262)

* Hint which data is where

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Add real life test case

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Fix the issue with emitting multiple lines

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Rename real_life_use_case_1 into two_lines_emit_with_continue_through

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Adjust the samples at two_lines_emit_with_continue_through for better formatting

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Add a two_lines_emit_with_halt_before test

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Fix an issue with emitting two messages at HaltBefore mode

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Add a comment on debug_assert

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Use pretty_assertions

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Fix another set of bugs and corrected two_lines_emit_with_halt_before test

- two_lines_emit_with_halt_before had hard to grasp example, which
  resulted in incorrect semantics, and effective a *wrong test*; the
  test samples were rewritten to ensure
- added more sample for the stashing logic tests, and hinted which test
  lines are going to be stashed for easier correspondence of the test logic
  with the code

Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruceg bruceg bruceg approved these changes

@binarylogic binarylogic Awaiting requested review from binarylogic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiline
2 participants
@MOZGIII @bruceg