Skip to content

Detox tests added #6

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

Merged

Conversation

DanielSanudo
Copy link
Collaborator

@DanielSanudo DanielSanudo commented Jun 7, 2019

Summary

  • Detox tests added for iOS and Android
  • Example app doesn't require anymore its own package.json
  • Example app dependencies now reference to the root node_modules

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

@DanielSanudo DanielSanudo requested a review from Swaagie June 7, 2019 12:23
Copy link
Collaborator

@Swaagie Swaagie left a comment

Choose a reason for hiding this comment

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

Nice work! 👏 Few minor comments

compileSdkVersion = 28
targetSdkVersion = 28
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did this need to be downgraded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what fixed the issue for running the detox tests in android-release. When using targetSdkVersion = 28 the app was getting installed but the tests weren't running.


// Set the default timeout
jest.setTimeout(120000);
jasmine.getEnv().addReporter(adapter);
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is jasmine coming from here? Does detox provide that on global by default?

Copy link

Choose a reason for hiding this comment

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

jest is actually using jasmine so it's coming from jest/adapter

Copy link
Collaborator

Choose a reason for hiding this comment

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

gotta luv test globals 😁

example/App.js Outdated
@@ -34,21 +34,40 @@ export default class App extends Component<Props> {
this.show('time');
}

mmddyyyy = (date) => {
var yyyy = date.getUTCFullYear();
var mm = date.getUTCMonth() < 9 ? '0' + (date.getUTCMonth() + 1) : (date.getUTCMonth() + 1); // getUTCMonth() is zero-based
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can probably be cleaned up a bit if the optional 0 is prepended after the actual, also would this display 010 if the returned month is 9? Also would prefer regular functions here as part of the class

Copy link

Choose a reason for hiding this comment

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

As this is an exampe app, can't we just use data-fn's to clean this up a little bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have ended up using moment. date-fns was giving me problems for timezones and it seems that there is no way of defining a timezone for the format function.

"resolved": "https://artifactory.secureserver.net/artifactory/api/npm/node-virt/core-js/-/core-js-3.1.2.tgz",
"integrity": "sha1-JUmiz7PKGl2FHJ94OOiygs7y87o=",
"version": "3.1.3",
"resolved": "https://artifactory.secureserver.net/artifactory/api/npm/node-virt/core-js/-/core-js-3.1.3.tgz",
Copy link

Choose a reason for hiding this comment

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

please use the npm.registry

example/App.js Outdated
@@ -34,21 +34,40 @@ export default class App extends Component<Props> {
this.show('time');
}

mmddyyyy = (date) => {
var yyyy = date.getUTCFullYear();
var mm = date.getUTCMonth() < 9 ? '0' + (date.getUTCMonth() + 1) : (date.getUTCMonth() + 1); // getUTCMonth() is zero-based
Copy link

Choose a reason for hiding this comment

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

As this is an exampe app, can't we just use data-fn's to clean this up a little bit?

@@ -1,13 +0,0 @@
module.exports = function (api) {
Copy link

Choose a reason for hiding this comment

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

Does the normal example still work after you deleted all of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, you just need to do npm run start and then npm run start:ios or npm run start:android. All the dependencies are now pointing to the root node_modules.

dateTimeText: {
fontSize: 16,
fontWeight: 'normal',
},
Copy link

Choose a reason for hiding this comment

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

Remove extra , -- but that also means that a linter didn't run, so can you run the linter over these files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have included the example folder for linting and it is requiring that ,. I guess that this is how it is defined in @react-native-community.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is part of the react-native-community eslint config, they like their comma's :)

@DanielSanudo DanielSanudo merged commit a33a29e into react-native-datetimepicker:master Jun 13, 2019
@DanielSanudo DanielSanudo deleted the detox_tests branch June 13, 2019 10:44
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.

4 participants