added system_time endpoint #361

SrinivasanTarget · 2016-01-25T11:57:54Z

https://github.com/appium/node-mobile-json-wire-protocol/issues/40

JaniJegoroff · 2016-01-25T12:15:47Z

lib/appium_lib/device/device.rb

@@ -15,6 +15,7 @@ module Device
        device_locked?:       'session/:session_id/appium/device/is_locked'
      },
      get:  {
+        get_device_time:        'session/:sessionId/appium/device/system_time',


We should follow Ruby coding conventions and not use get prefixes in method names. So the better name would be device_time.

Just wondering since all the other endpoints are session/:session_id but this one is session/:sessionId. Is this correct?

@JaniJegoroff https://github.com/appium/node-mobile-json-wire-protocol/blob/master/lib/routes.js#L294 It should be "sessionId" not "session_id".

Just wondering since all the other endpoints are session/:session_id but this one is session/:sessionId. Is this correct?

@JaniJegoroff https://github.com/appium/node-mobile-json-wire-protocol/blob/master/lib/routes.js#L294 It should be "sessionId" not "session_id".

@bootstraponline What do you think about this? Quickly looking the code there's some kind of patching done for session_id's.

:sessionId is wrong. routes.js is JavaScript, not Ruby. Device methods in Ruby are specified like this:

session/:session_id/appium/device/

bootstraponline · 2016-01-25T14:34:20Z

Hi, this should have a test.

SrinivasanTarget · 2016-01-25T16:34:19Z

Will update the tests as well soon

SrinivasanTarget · 2016-01-29T18:17:34Z

@bootstraponline @JaniJegoroff Could you please help me in tests i added, on what value i can assert the device date time?

bootstraponline · 2016-01-29T18:22:14Z

Could you please help me in tests i added, on what value i can assert the device date time?

I'm assuming the date time is reported in a certain format that can be matched generically with the use of a regular expression.

SrinivasanTarget · 2016-01-29T18:24:23Z

@bootstraponline But that changes with the time zone right? sample result: Fri Jan 29 23:53:35 IST 2016

bootstraponline · 2016-01-29T18:31:52Z

> 'Fri Jan 29 23:53:35 IST 2016'.match /\d{2}:\d{2}:\d{2}/
=> #<MatchData "23:53:35">

All we care about is that the server returned something reasonable, it doesn't matter what. The actual device time tests should already exist in the appium node.js repo and hopefully they're mocking the device time.

bootstraponline · 2016-01-29T18:35:31Z

This should work:

expect(device_time).to match(/\d{2}:\d{2}:\d{2}/)

bootstraponline · 2016-01-29T18:40:26Z

Please squash to one commit.

git reset --soft head~5
git commit -am "Add device_time"
git push -f

bayandin · 2016-01-29T18:41:05Z

Why not Date.parse(...)?

SrinivasanTarget · 2016-01-29T18:43:54Z

Date.parse could again needs the regular expression.Both does the same right?

bootstraponline · 2016-01-29T18:44:30Z

Why not Date.parse(...)?

because all we care about is that the server responded with something resembling a date. If the appium server is sending invalid dates then that should be detected by the JavaScript tests in the server repo, not the Ruby bindings. I don't think appium server has any guarantees that the returned date will work with Ruby's Date.parse.

The idea is for the test to be resilient if/when appium decides to change their date format.

SrinivasanTarget · 2016-01-29T18:45:22Z

Squashed now @bootstraponline

bootstraponline · 2016-01-29T18:46:31Z

Date.parse could again needs the regular expression.Both does the same right?

No, it doesn't need a regex.

> Date.parse 'Fri Jan 29 23:53:35 IST 2016'
=> #<Date: 2016-01-29 ((2457417j,0s,0n),+0s,2299161j)>

It means if appium gives us an invalid date then the Ruby test will fail, which I think is wrong. We're not testing that appium returns valid dates since that's beyond the scope of the client bindings.

bootstraponline · 2016-01-29T18:47:20Z

Squashed now @bootstraponline

Looks good to me. Unless @bayandin has a good argument for why we should validate the date at the Ruby layer then I'm ready to merge.

SrinivasanTarget · 2016-01-29T18:47:49Z

Ok Got it.Thanks @bootstraponline

bayandin · 2016-01-29T18:49:52Z

@bootstraponline I'm completely agree with you, but it seems expect(device_time).to match(/\d{2}:\d{2}:\d{2}/) is more strict check than Date.parse(device_time). Maybe is enough just check that server returns a string.

bootstraponline · 2016-01-29T18:53:37Z

it seems expect(device_time).to match(/\d{2}:\d{2}:\d{2}/) is more strict check than Date.parse(device_time)

oh, I didn't think of it like that. I agree Date.parse is better then.

Maybe is enough just check that server returns string.

I think we'll want to be more strict than just a string so we'll go with Date.parse. Thanks for commenting!

@SrinivasanTarget the test can be updated to just be Date.parse(device_time). If device time returns an invalid date then parse will raise an ArgumentError which will fail the test.

bootstraponline · 2016-01-29T19:00:43Z

@SrinivasanTarget needs to be squashed to one commit :) then it'll be ready to merge.

SrinivasanTarget · 2016-01-29T19:01:55Z

Yup Squashed @bootstraponline

added system_time endpoint

bootstraponline · 2016-01-29T19:02:47Z

merged! Thanks for contributing to appium.

SrinivasanTarget · 2016-01-29T19:03:31Z

Thanks @bootstraponline @bayandin

bayandin · 2016-01-29T19:04:34Z

Just wondering, is there somewhere require 'date'? :)
@SrinivasanTarget thanks!

SrinivasanTarget · 2016-01-29T19:39:37Z

Yes it has to be.Shit missed it.I saw in one example now!

bootstraponline · 2016-01-29T19:39:49Z

Just wondering, is there somewhere require 'date'? :)

require 'date' is in the main Rakefile (which is used to run the tests). Not sure why it works since I'd expect it to have to be in the test Rakefile as well. It does work though, I ran the test. It looks like the endpoint isn't active in the server yet.

Selenium::WebDriver::Error::WebDriverError: unexpected response, code=404, content-type="text/plain"
That URL did not map to a valid JSONWP resource

I released 8.0.2 to Rubygems.

bootstraponline · 2016-01-29T19:43:07Z

$ rake ios['device/device']
bundle exec ruby ./lib/run.rb ios "device/device"
Loading one test: /ruby_lib/ios_tests/lib/ios/specs/device/device.rb
device_time | 1 | specs/device/device.rb:3
  t 'device_time' do
    Date.parse 'Jan'
    Date.parse(device_time)


Finished in 0s

  1) Error:
device/device#test_0001_device_time:
Selenium::WebDriver::Error::WebDriverError: unexpected response, code=404, content-type="text/plain"
That URL did not map to a valid JSONWP resource

SrinivasanTarget · 2016-01-29T19:44:33Z

What is the reason for this error @bootstraponline ?
Are we running in simulator?device_time on ios supports only real devices only

bootstraponline · 2016-01-29T19:45:25Z

@SrinivasanTarget

The appium server doesn't recognize the endpoint.

info: --> GET /wd/hub/session/bdd2c145-2298-4a18-bad2-47618005a952/appium/device/system_time {}
info: [debug] Responding to client that we did not find a valid resource

SrinivasanTarget · 2016-01-29T19:46:21Z

Usually how does an endpoint been activated in mjsonwp/appium?

bootstraponline · 2016-01-29T19:48:55Z

My guess is the appium devs only added this new endpoint for appium 1.5 and it wasn't accepted into 1.4.

SrinivasanTarget · 2016-01-29T19:49:40Z

Yes i raised PR only on 1.5 branch

bootstraponline · 2016-01-29T19:50:30Z

ok, it should work in 1.5 then (I don't use that for testing hence the failure).

SrinivasanTarget · 2016-01-29T19:51:09Z

Ok Thanks @bootstraponline

SrinivasanTarget · 2016-01-29T19:51:58Z

will run in 1.5 and check

JaniJegoroff · 2016-01-29T20:21:15Z

I don't see endpoint in 1.5 yet.

Janis-iMac:~ janijegoroff$ appium
[Appium] Welcome to Appium v1.5.0-beta11 (REV d19c85fa5aa8359ea81f5fbe8c3fe977b45ea526)
[Appium] Appium REST http interface listener started on 0.0.0.0:4723

Janis-iMac:ios_tests janijegoroff$ arc
[1] pry(main)> device_time
Selenium::WebDriver::Error::UnknownError: Method has not yet been implemented

bootstraponline · 2016-01-29T20:25:10Z

You probably need the latest revision from the 1.5 branch.

JaniJegoroff · 2016-01-29T20:36:10Z

I tried against revision whatever is installed with command npm install -g appium@beta.

bootstraponline · 2016-01-29T20:36:54Z

I think you'd have to use nightly and not beta.

JaniJegoroff · 2016-01-29T22:55:06Z

Actually, it looks like beta is in sync with 1.5 branch commits.

bootstraponline · 2016-01-29T23:19:13Z

really? d19c85fa5aa8359ea81f5fbe8c3fe977b45ea526 isn't the latest though.

JaniJegoroff · 2016-01-29T23:29:39Z

I was looking the recent changes. I updated beta version and appium-android-driver@1.6.1 update was fetched: appium/appium@5661929

Janis-iMac:~ janijegoroff$ npm install -g appium@beta
npm WARN deprecated babel-core@5.8.24: Babel 5 is no longer being maintained. Upgrade to Babel 6.
/usr/local/bin/appium -> /usr/local/lib/node_modules/appium/build/lib/main.js
/usr/local/bin/authorize-ios -> /usr/local/lib/node_modules/appium/node_modules/.bin/authorize-ios
/usr/local/lib
└─┬ appium@1.5.0-beta11
  └── appium-android-driver@1.6.1

bootstraponline · 2016-01-29T23:34:37Z

awesome! good to know that beta is tracking the 1.5 branch.

JaniJegoroff reviewed Jan 25, 2016
View reviewed changes

SrinivasanTarget force-pushed the master branch from e9fe84d to c5637a5 Compare January 29, 2016 18:42

Add device_time

02a3f5f

SrinivasanTarget force-pushed the master branch from 8b64265 to 02a3f5f Compare January 29, 2016 19:01

bootstraponline added a commit that referenced this pull request Jan 29, 2016

Merge pull request #361 from SrinivasanTarget/master

92be077

added system_time endpoint

bootstraponline merged commit 92be077 into appium:master Jan 29, 2016

bayandin mentioned this pull request Jan 29, 2016

Implement /wd/hub/session/:sessionId/appium/device/system_time endpoint #360

Closed

Uh oh!

added system_time endpoint #361

added system_time endpoint #361

Uh oh!

Conversation

SrinivasanTarget commented Jan 25, 2016

Uh oh!

JaniJegoroff Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

JaniJegoroff Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

SrinivasanTarget Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

JaniJegoroff Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

bootstraponline Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

bootstraponline commented Jan 25, 2016

Uh oh!

SrinivasanTarget commented Jan 25, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

bayandin commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bayandin commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bayandin commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!

SrinivasanTarget commented Jan 29, 2016

Uh oh!

bootstraponline commented Jan 29, 2016

Uh oh!