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

Fixes CB-7781 #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

aetchegoyen
Copy link

With this change the issue reported in https://issues.apache.org/jira/browse/CB-7781.
Tested and working perfectly now while uninstalling plugin

With this change the issue reported in https://issues.apache.org/jira/browse/CB-7781.
Tested and working perfectly now while uninstalling plugin
@dseravalli
Copy link

Is this going to be merged in?

@mreinstein
Copy link
Collaborator

why do the tests fail with this patch?

@TooTallNate
Copy link
Owner

Needs a regression test.

@Bnaya
Copy link

Bnaya commented Oct 15, 2015

waiting forward for this,
How's the regression test should looks like?

@absynce
Copy link

absynce commented Nov 10, 2015

FWIW I manually applied this patch and it allowed me to remove a plugin in Ionic/Cordova.

@Bnaya
Copy link

Bnaya commented Nov 10, 2015

same here

@mreinstein
Copy link
Collaborator

I'm glad that manually applying the patch fixes your problem, but it would be nice if this pull request had at least one test demonstrating the fix.

Submitting patches without tests isn't acceptable.

@benallfree
Copy link

@mreinstein Here you go, for test/build.js:

    it('should create an empty plist XML string from null input', function () {
      var xml = build();
      assert.strictEqual(xml, multiline(function () {/*
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0"/>
*/}));
    });

@mreinstein
Copy link
Collaborator

I've finally got the testing infrastructure somewhat working again. If you re-submit a clean PR with tests included we can merge this now.

Sorry for the painfully long response time on this. 🤦

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.

None yet

7 participants