webdriverio-boneyard / webdrivercss

Regression testing tool for WebdriverIO v4
http://webdriver.io
MIT License
615 stars 106 forks source link

How to send more data in syncUp/ syncDown #55

Closed amitaibu closed 9 years ago

amitaibu commented 9 years ago

Currently the syncUp/ syncDown hard codes the POST values. I'd like to extend it, so I could for example send a CSRF-Token, or other arbitrary info upon syncing.

Is there a recommended way for doing this? (I can write a PR, just need some guidance on the best practice here)

christian-bromann commented 9 years ago

The syncUp/syncDown operations are currently rudimental implementations and can get improved in any kind of way. @TheSavior and I where thinking of providing multiple storage options (e.g. local storage, dropbox, amazon cloud or within a git model). I am totally excited to work on those thinks but unfortunately I have limited time as I am currently pounding out v2.5.0 and work on bunch of other thinks like documentation etc.

My point is if you could do a PR that would be the optimal way. As guidance I would like to advise you to build some kind of storage interface so we can easily adopt more storage options or at least we can easily reuse the code as local storage option.

Thanks in advance for any help ;-)

elicwhite commented 9 years ago

I'm not a big proponent of adding storage type of things as part of webdrivercss core. Personally I see that functionality being more of a part of the admin panel.

I think being able to send an object to sync() that would be sent onto the api server would be huge. I could easily imagine sending up the browser name / browser version or git branch or something like that.

amitaibu commented 9 years ago

I'm not a big proponent of adding storage type of things as part of webdrivercss core.

I agree. I think the webdrivercss core should provide the mechanism for "proviers" to register themself. In fact Applitools for example would be the first candidate to move out of the core, and use such a registration system.

I imagine something like this:

var WebdriverIO = require('webdriverio'),
    WebdriverCSS = require('webdrivercss');

    WebdriverCSSSomeProvider = require('webdrivercss-some-provider');

WebdriverCSS.init(client);

// Add provider.
WebdriverCSSSomeProvider.set({
   apiKey: 'foo',
   arbitraryKey: {}
});

// Register multiple providers.
WebdriverCSS.registerProvider(WebdriverCSSSomeProvider.get());
WebdriverCSS.registerProvider(anotherProvider.get());

Another part of this would be #37 - which will allow the providers to hook and act when they decide, not only upon sync()

christian-bromann commented 9 years ago

@amitaibu sounds like a good idea. It might require some structural changes though. But it would be the way to go. As I was talking about the interface I was referring to the adminpanel project as it does all the work of taking care of the baseline images and stuff. There are two options:

  1. we create a more sophisticated sync algorithm that allows user to send additional data (csrf tokens or maybe which storage system is desired) and webdrivercss-adminpanel will do all the work or
  2. we create specific storage provider that can be used with WebdriverCSS. This would be a more universal approach and would decouple the relationship between WebdriverCSS and the adminpanel.

I would favor option 2 as it will allow user to sync the image repository with all these different storage providers without necessarily having the adminpanel running. What do you guys think?

amitaibu commented 9 years ago

I think the admin panel is a great starting point, but one that should only be an example for a client consuming the API.

It might require some structural changes though

Yes, probably ;) Another option is that instead of the magic sync(), we can have a syncUp callback as webdrivercss fourth callback - so after every successful image, one would be able to upload to any provider they want.

e.g.

.webdrivercss('foo', {name: 'foo'}, function response(err, res), function syncUp(res) {

  // Iterate over the images.
  // Send each image with some metadata
});

So that one won't break the API.

amitaibu commented 9 years ago

erase that, I meant:

var syncUp = function syncUp(res) {

  // Iterate over the images.
  // Send each image with some metadata
}

.webdrivercss('foo', {name: 'foo', syncUp: syncUp}, function response(err, res));
elicwhite commented 9 years ago

For my use case, I wouldn't want to be specifying the syncUp callback on every call to webdrivercss. That means it would either need to go into init or as an object to sync().

amitaibu commented 9 years ago

That is also possible, as long as we allow sending arbitrary metadata with the POST

‫ב-16 בפבר 2015, בשעה 19:55, ‏Eli White notifications@github.com כתב/ה:‬

For my use case, I wouldn't want to be specifying the syncUp callback on every call to webdrivercss. That means it would either need to go into init or as an object to sync().

— Reply to this email directly or view it on GitHub.

elicwhite commented 9 years ago

Bumping this again, it would be extremely useful to us. @amitaibu, are you planning on tackling this? I would gladly accept a PR with this functionality.

I think specifying the data in the sync function is the right approach since that is when the call is made. So the API would look like:

client
  .init()
  .sync()
  .url('http://example.com')
  .webdrivercss('home-page', [{
    name: 'navbar',
    elem: '#navbar'
  }])
  .sync({
    browser: 'chrome',
    version: '35',
    os: 'windows'
  })
  .end();
amitaibu commented 9 years ago

@amitaibu, are you planning on tackling this?

My nodejs isn't as good as my PHP, but I can try :)

However I think the example you gave for sync, isn't enough as it's missing some important metadata:

  .sync({
    browser: 'chrome',
    version: '35',
    os: 'windows'
  })

I think that a good flow of syncUp would be:

So it would be easier for the system I'm trying to build (which is webdrivercss-admin with access and notifications built in) to "understand" which image was uploaded.

elicwhite commented 9 years ago

Ah, I think I have some understanding of what you are trying to build now.

My feeling is that in order to provide the ability to get full results of the diffed images and send those up to a server has a lot deeper changes to be made to webdrivercss. I've created issue #58 for that.

I think I'd still like to start with a simple pass through of an object from sync sent as post data to the api server since that is significantly less work for still a lot of value.

amitaibu commented 9 years ago

send those up to a server has a lot deeper changes to be made to webdrivercss

Maybe, but after some more thinking I think it would be enough just to stay with the json part:

Create a WebdrivercssResult.json file with all the res and the client's capabilities So then that json file will be added to that tar in the sync()

elicwhite commented 9 years ago

We are really looking to solve two very different goals here. I'm trying to send up the git branch and browser details which are all things I have at run time and could just be passed to an object on the sync up call.

It seems like you need a reporter output for webdrivercss (core changes to calculate and build up, then write just before sync happens when it knows all of the screenshots have been generated) that would then be sent up during sync as well.

I do think it would be valuable to have that kind of reporter, but it seems like a separate issue from being able to send up additional arbitrary data in sync (which is what I'm trying to do). Do you agree on the difference?

amitaibu commented 9 years ago

No, I think it's same goals, as I also want to have the Git commit and browser :)

‫ב-22 בפבר 2015, בשעה 02:00, ‏Eli White notifications@github.com כתב/ה:‬

We are really looking to solve two very different goals here. I'm trying to send up the git branch and browser details which are all things I have at run time and could just be passed to an object on the sync up call.

It seems like you need a reporter output for webdrivercss (core changes to calculate and build up, then write just before sync happens when it knows all of the screenshots have been generated) that would then be sent up during sync as well.

I do think it would be valuable to have that kind of reporter, but it seems like a separate issue from being able to send up additional arbitrary data in sync (which is what I'm trying to do). Do you agree on the difference?

— Reply to this email directly or view it on GitHub.

amitaibu commented 9 years ago

In other words, on top of the run time variables, I'm also looking for a per image info.

elicwhite commented 9 years ago

Great. So it sounds like this is three tasks, correct?

elicwhite commented 9 years ago

@amitaibu, I actually think you could get the data you want right now.

Since every call to webdrivercss takes a callback that is provided information about the images taken, you could do something like this:

var imageData = [];

client
.init()
.url('http://example.com')
.webdrivercss('page name', elementsToCapture
], function(err, res) {
    // add the image data you want from res into the imageData array
})
.sync({
  imageData: imageData,
  browser: browserName,
  os: osName
})

With a system like that, you wouldn't need any additional core changes and we would both be able to utilize a simple pass through of data from sync up to the server.

If you follow this pattern a bunch, you could easily provide a reusable callback that creates the imageData array/object you need.

amitaibu commented 9 years ago

Yes, you are right, this is something I've actually started looking at :)

amitaibu commented 9 years ago

@TheSavior here's my hackish solution, which I've developed as a POC. It's a little ugly, but on the other hand it works without changing webdrivercss ;)

elicwhite commented 9 years ago

Heh, this is mine. We clearly came to the same hackish solution:

client
  .init()
  .url(website)
  .webdrivercss('elements', [{
    name: 'button',
    elem: '#button'
  }, {
    name: 'form',
    elem: '#form'
  }])
  .call(upload)
  .end();

function upload() {
  git.branch(function(branchName) {
    // branchName = 'master';

    new targz()
      .compress(screenshotRoot, screenshotRoot + '.tar.gz', function(err) {
        if (err) {
          throw new Error(err);
        }

        var args = {
          url: api + 'upload',
        };

        var r = request.post(args, function(err, httpResponse, body) {
          if (err || (httpResponse && httpResponse.statusCode !== 200)) {
            throw new Error(err || body);
          }
        });

        var form = r.form();
        form.append('branchName', branchName);
        form.append('gz', fs.createReadStream(screenshotRoot + '.tar.gz'));

      });
  });
}
amitaibu commented 9 years ago

:)

I'll close this issue, as obviously there are ways to achieve our goal(s).

amitaibu commented 9 years ago

fyi - here's a post about Shoov the new UI regression tool we're working on - with a first implementation with webdrivercss :)

christian-bromann commented 9 years ago

@amitaibu that's sooo awesome. I love to see that people starting to create an open source eco system. Keep up the good work and I promise that I will circle back to webdrivercss as soon as possible.

amitaibu commented 9 years ago

:+1:

el3ment commented 9 years ago

+1 Amitai!

Robert

On Wed, Apr 1, 2015 at 11:37 AM, Amitai Burstein notifications@github.com wrote:

[image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/webdriverio/webdrivercss/issues/55#issuecomment-88568677 .