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

code: merge modifications back to the original file #32

Merged
merged 4 commits into from May 12, 2019

Conversation

lonng
Copy link
Contributor

@lonng lonng commented May 10, 2019

Signed-off-by: Lonng chris@lonng.org

What problem does this PR solve?

If we do the following steps:

  1. failpoint-ctl enable
  2. modify some code
  3. failpoint-ctl disable

We will lose modifications in step2

What is changed and how it works?

Merge modifications back to the original file

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

Related changes

Signed-off-by: Lonng <chris@lonng.org>
@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #32 into master will decrease coverage by 0.538%.
The diff coverage is 69.2307%.

@@               Coverage Diff                @@
##             master        #32        +/-   ##
================================================
- Coverage   86.9318%   86.3938%   -0.5381%     
================================================
  Files             7          7                
  Lines           880        904        +24     
================================================
+ Hits            765        781        +16     
- Misses           73         77         +4     
- Partials         42         46         +4

@lonng lonng added feature New feature S:PTAL labels May 10, 2019
@lonng lonng changed the title code: merge modifications back to original file code: merge modifications back to the original file May 10, 2019
code/restorer.go Outdated
patcher := diffmatchpatch.New()
diffs := patcher.DiffMain(buffer.String(), string(rewritedContent), true)
patches := patcher.PatchMake(diffs)
pathedContent, _ := patcher.PatchApply(patches, string(originContent))
Copy link
Contributor

Choose a reason for hiding this comment

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

The Apply could error, no? Like if I deleted a failpoint.Eval invocation on the enabled file it will likely fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@kennytm kennytm May 10, 2019

Choose a reason for hiding this comment

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

Could we consider the apply failed if the returned array contained at least one false entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

Signed-off-by: Lonng <chris@lonng.org>
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Please add a test case showing that if we do any change to the failpoint.Eval call itself it will fail. Rest LGTM

Signed-off-by: Lonng <chris@lonng.org>
Copy link
Collaborator

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added S:LGT2 and removed S:LGT1 labels May 12, 2019
@lonng lonng merged commit 30cc743 into master May 12, 2019
@lonng lonng deleted the lonng/restore-merge branch May 12, 2019 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants