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

Fix10 - RegexBasedInterpolatorTest fails when building with maven 3.4.0-SNAPSHOT #11

Merged
merged 10 commits into from
May 25, 2017

Conversation

belingueres
Copy link
Contributor

Hi,
I've prepared this PR to solve the issue. I just used $JAVA_HOME when testing with a real env var. Used mocks on the env var Map whenever is not the focus of the test.
Gabriel

ReflectionValueExtractor version with capacity to parse expressions with
arrays, lists and maps.
…t. The reason is that I've made a mistake and created a unique PR to solve several issues, which is a bad practice.
…ng with maven 3.4.0-SNAPSHOT

Tests fails because the $HOME env var is no longer defined inside mvn.cmd and is not found (some tests pass even if the env var is not found!).

- Changed the OperatingSystemUtils to allow mocking the Map of env vars.
- Fixed tests to use mocking whenever the env vars are not the focus of the unit test.
- When testing with a REAL env var, it is chosen to be $JAVA_HOME since it is required to be present to run mvn script.
- Added tests to EnvarBasedValueSource and fixed.EnvarBasedValueSource.
@michael-o
Copy link
Member

Please rebase your branch first and then apply your chnges within a single commit.

…ng with maven 3.4.0-SNAPSHOT

Tests fails because the $HOME env var is no longer defined inside mvn.cmd and is not found (some tests pass even if the env var is not found!).

- Changed the OperatingSystemUtils to allow mocking the Map of env vars.
- Fixed tests to use mocking whenever the env vars are not the focus of the unit test.
- When testing with a REAL env var, it is chosen to be $JAVA_HOME since it is required to be present to run mvn script.
- Added tests to EnvarBasedValueSource and fixed.EnvarBasedValueSource.
@belingueres
Copy link
Contributor Author

Rebased.
Thanks!
Gabriel

@michael-o
Copy link
Member

Can you again resolve the merge issue? I'll test and merge.

# Conflicts:
#	src/test/java/org/codehaus/plexus/interpolation/fixed/FixedStringSearchInterpolatorTest.java
@belingueres
Copy link
Contributor Author

Merged.

@michael-o
Copy link
Member

You have now eight commits. I wanted a single commit applyable to master.

@belingueres
Copy link
Contributor Author

I can't update the PR with the squashed commit. Any ideas how to solve it?

@michael-o
Copy link
Member

Create a patch from your change, reset your branch to origin/master, apply patch, resolve issues.

@belingueres
Copy link
Contributor Author

Sorry can't make it work....

…lation into Fix10

# Conflicts:
#	src/test/java/org/codehaus/plexus/interpolation/EnvarBasedValueSourceTest.java
#	src/test/java/org/codehaus/plexus/interpolation/fixed/EnvarBasedValueSourceTest.java
@olamy
Copy link
Member

olamy commented May 17, 2017

well the github merge button offer an option to squash and merge so @michael-o can do it while merging.

@michael-o
Copy link
Member

michael-o commented May 17, 2017

@olamy This won't help. If you rebase this branch there are a lot of merge conflicts. It is easier to rework the patch on top of master rather than reconcile this.

@olamy
Copy link
Member

olamy commented May 18, 2017

@michael-o conflicts? AFAICS github UI doesn't show any conflicts? Do I miss something?

@belingueres
Copy link
Contributor Author

I checked the "Allow edits from maintainers." option...don't know if it does something useful.

@michael-o
Copy link
Member

michael-o commented May 18, 2017

@olamy

mosipov@mikaw10 MINGW64 /d/Entwicklung/Projekte/plexus-interpolation (master)
$ git checkout -B pr-11
Switched to a new branch 'pr-11'

mosipov@mikaw10 MINGW64 /d/Entwicklung/Projekte/plexus-interpolation (pr-11)
$ git merge belingueres/Fix10
Updating 0d3c1d2..b7fcd97
Fast-forward
 .../interpolation/EnvarBasedValueSource.java       |  9 ++
 .../interpolation/fixed/EnvarBasedValueSource.java |  9 ++
 .../interpolation/os/OperatingSystemUtils.java     | 44 +++++++++-
 .../interpolation/EnvarBasedValueSourceTest.java   | 97 +++++++++++++++++++++
 .../interpolation/RegexBasedInterpolatorTest.java  | 24 +++++-
 .../StringSearchInterpolatorTest.java              | 25 +++++-
 .../fixed/EnvarBasedValueSourceTest.java           | 98 ++++++++++++++++++++++
 .../fixed/FixedStringSearchInterpolatorTest.java   | 25 +++++-
 8 files changed, 319 insertions(+), 12 deletions(-)
 create mode 100644 src/test/java/org/codehaus/plexus/interpolation/EnvarBasedValueSourceTest.java
 create mode 100644 src/test/java/org/codehaus/plexus/interpolation/fixed/EnvarBasedValueSourceTest.java

mosipov@mikaw10 MINGW64 /d/Entwicklung/Projekte/plexus-interpolation (pr-11)
$ git rebase master
First, rewinding head to replay your work on top of it...
Applying: Fixed MultiDelimiterStringSearchInterpolator escape String code
Using index info to reconstruct a base tree...
M       src/main/java/org/codehaus/plexus/interpolation/multi/MultiDelimiterStringSearchInterpolator.java
M       src/test/java/org/codehaus/plexus/interpolation/multi/MultiDelimiterStringSearchInterpolatorTest.java
.git/rebase-apply/patch:18: space before tab in indent.
                                --startEscapeIdx;
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging src/test/java/org/codehaus/plexus/interpolation/multi/MultiDelimiterStringSearchInterpolatorTest.java
CONFLICT (content): Merge conflict in src/test/java/org/codehaus/plexus/interpolation/multi/MultiDelimiterStringSearchInterpolatorTest.java
Auto-merging src/main/java/org/codehaus/plexus/interpolation/multi/MultiDelimiterStringSearchInterpolator.java
CONFLICT (content): Merge conflict in src/main/java/org/codehaus/plexus/interpolation/multi/MultiDelimiterStringSearchInterpolator.java
error: Failed to merge in the changes.
Patch failed at 0001 Fixed MultiDelimiterStringSearchInterpolator escape String code
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

@olamy
Copy link
Member

olamy commented May 23, 2017

no problem here (see the branch called merge-pr-11 )

mb-olamy:plexus-interpolation olamy$ git fetch origin pull/11/head:pr-11
remote: Counting objects: 109, done.
remote: Compressing objects: 100% (16/16), done.
remote: Total 109 (delta 33), reused 59 (delta 29), pack-reused 44
Receiving objects: 100% (109/109), 23.69 KiB | 0 bytes/s, done.
Resolving deltas: 100% (34/34), completed with 12 local objects.
From https://github.com/codehaus-plexus/plexus-interpolation
 * [new ref]         refs/pull/11/head -> pr-11
mb-olamy:plexus-interpolation olamy$ git checkout pr-11
Switched to branch 'pr-11'
mb-olamy:plexus-interpolation olamy$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
mb-olamy:plexus-interpolation olamy$ git checkout -b merge-pr-11
Switched to a new branch 'merge-pr-11'
mb-olamy:plexus-interpolation olamy$ git merge pr-11
Updating 0d3c1d2..b7fcd97
Fast-forward
 src/main/java/org/codehaus/plexus/interpolation/EnvarBasedValueSource.java                   |  9 +++++++++
 src/main/java/org/codehaus/plexus/interpolation/fixed/EnvarBasedValueSource.java             |  9 +++++++++
 src/main/java/org/codehaus/plexus/interpolation/os/OperatingSystemUtils.java                 | 44 ++++++++++++++++++++++++++++++++++++++++++--
 src/test/java/org/codehaus/plexus/interpolation/EnvarBasedValueSourceTest.java               | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/test/java/org/codehaus/plexus/interpolation/RegexBasedInterpolatorTest.java              | 24 +++++++++++++++++++++---
 src/test/java/org/codehaus/plexus/interpolation/StringSearchInterpolatorTest.java            | 25 ++++++++++++++++++++++---
 src/test/java/org/codehaus/plexus/interpolation/fixed/EnvarBasedValueSourceTest.java         | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/test/java/org/codehaus/plexus/interpolation/fixed/FixedStringSearchInterpolatorTest.java | 25 +++++++++++++++++++++----
 8 files changed, 319 insertions(+), 12 deletions(-)
 create mode 100644 src/test/java/org/codehaus/plexus/interpolation/EnvarBasedValueSourceTest.java
 create mode 100644 src/test/java/org/codehaus/plexus/interpolation/fixed/EnvarBasedValueSourceTest.java
mb-olamy:plexus-interpolation olamy$ 

@michael-o
Copy link
Member

@olamy Checked the branch. It now contains 4 ugly merge commits. Barely impossible to read the changes introduced.

@olamy
Copy link
Member

olamy commented May 23, 2017

well maybe @belingueres had some issues while rebasing.
But looking at the diff via the github ui everything looks good.

@belingueres
Copy link
Contributor Author

Thanks you guys for your help. I think the problem was the squashing...I must be doing something wrong. Maybe you could squash the last changes into one commit in the merge-pr-11 branch and post the sequence of commands to illustrate how to do it right?

@michael-o
Copy link
Member

I tried that locally with rebase. It produces non-mergable commits which must be resolved locally.

@olamy
Copy link
Member

olamy commented May 24, 2017

Ok so as said previously local merge works for me and github ui doesn't mention any conflict.
Sorry @belingueres I have to give here up as it looks impossible to convince @michael-o .....
I'm tired to waste my time....

@michael-o
Copy link
Member

@olamy Can you squash the branch into one commit and push?

@olamy olamy merged commit d187343 into codehaus-plexus:master May 25, 2017
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.

3 participants