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

[TIMOB-25459] iOS: Change "toImage" callback-parameter to match Android / Windows #9583

Merged
merged 3 commits into from Nov 9, 2017
Merged

[TIMOB-25459] iOS: Change "toImage" callback-parameter to match Android / Windows #9583

merged 3 commits into from Nov 9, 2017

Conversation

hansemannn
Copy link
Collaborator

JIRA: https://jira.appcelerator.org/browse/TIMOB-25459

Trying to fix the commented unit-test as well.

@hansemannn hansemannn added this to the 7.0.0 milestone Nov 3, 2017
@hansemannn hansemannn self-assigned this Nov 3, 2017
@build build added the ios label Nov 3, 2017
@build
Copy link
Contributor

build commented Nov 3, 2017 •
edited

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.
🚫

Tests have failed, see below for more information.

Tests:

Classname Name Time Error
android.Titanium.Network.HTTPClient clearCookieUnaffectedCheck 60.051 Error: timeout of 60000ms exceeded

Generated by 🚫 dangerJS

vijaysingh-axway
vijaysingh-axway approved these changes Nov 8, 2017
View reviewed changes
Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

CR and FT passed.

@build build added the ios label Nov 9, 2017
@mukherjee2 mukherjee2 self-requested a review November 9, 2017 17:39
mukherjee2
mukherjee2 approved these changes Nov 9, 2017
View reviewed changes
Copy link
Contributor

@mukherjee2 mukherjee2 left a comment

Choose a reason for hiding this comment

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

Passed FR with:
Node Version: 6.11.5
NPM Version: 3.10.10
Mac OS: 10.13
Appc CLI: 7.0.0-master.6
Appc CLI NPM: 4.2.11-2
Titanium SDK version: 7.0.0 locally built w/ PR/9583
Appcelerator Studio vers 4.10.0
Xcode 9.1/iOS 11.1 iPhone 7 Plus
Android 7.1.2 Pixel
I used the test case provided by Hans in the ticket, and verified that the app works on both Android and iOS.

@mukherjee2
Copy link
Contributor

mukherjee2 commented Nov 9, 2017 •
edited

@hansemannn there are test failures and Merge is grayed out. Can you help unblock? FR passed, and I am ready to have this PR merged.

@sgtcoolguy sgtcoolguy merged commit 5744034 into tidev:master Nov 9, 2017
@ewanharris ewanharris mentioned this pull request Nov 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mukherjee2 mukherjee2 mukherjee2 approved these changes

@vijaysingh-axway vijaysingh-axway vijaysingh-axway approved these changes

Assignees

@hansemannn hansemannn

Labels
BREAKING CHANGES ⚠️ improvement in-qe-testing 🕵 ios needs tests 🚨
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hansemannn @build @mukherjee2 @vijaysingh-axway @sgtcoolguy