@fenduru I'm not sure about decorator approach.
From your PR it seems that you collect some functions which, when called, must modify somehow SuiteBuilder API. I don't think this is a right approach to redefine standard API on the fly, it might be better to extend it somehow with your custom actions. The reasons are following:

• modified API may not work in a way gemini expecting. This will most likely lead to strange results or errors which will be hard to investigate.
• modified API need from us revealing private fields to public (suite for example) which is also not seems to be a good idea.
• modified API will obviously not work as described in documentation and may confuse someone, who is not familiar with your custom tweaks.
  Instead of modifying existing API I'd rather pick a way with extending it with custom actions which may use standard APIs and build some new behavior on top of it.

@SwinX the reason I went with the decorator pattern is because I want the plugin I'm going to write to change the way that Gemini acts, but not change the way that I need to write tests. My use case is to automatically run the same test multiple times, changing the theme between each capture.

My plugin wraps the suite.capture function to generate multiple images per capture call. It doesn't do anything with private API, and simply implements the identical interface that capture does. That said, the this.suite property might actually be useful in some plugins so that properties can be inherited by nested suites (in the same way that suite.setCaptureElements('foo') does this.suite.captureElements = 'foo').

I think that any plugin decorating the suite shouldn't modify public API, but that doesn't mean that they shouldn't be able to wrap the API so that it can enhance existing functions.

I want to write my tests like this:

gemini.suite('foo', function(suite) {
  // this test doesn't need to know/care that it will be tested in multiple themes
  // it is valid regardless of if my plugin is loaded
  suite.capture('bar')
          .capture('baz');
});

not

gemini.suite('foo', function(suite) {
  suite.captureAllThemes('bar')
          .captureAllThemes('baz');
});

@SwinX any further thoughts on this?

@fenduru sorry for delayed response.

I want the plugin I'm going to write to change the way that Gemini acts, but not change the way that I need to write tests.

Sorry, but it seems like not a good approach. The first question is why you need to keep existing API if you need different behavior from it? If you need to capture theme, then more obvious is some custom command like captureTheme.

As I told earlier, Gemini expects some standard behavior from suite APIs. If it will not act as expected, this will lead to Gemini malfunction.
For your specific case you would like to write smth like this in updated capture command:

gemini.decorateSuite(function(builder) {
    builder.oldCapture = builder.capture;
    builder.capture = function() {
        builder.oldCapture('theme1', function(actions) {
            actions.executeJS(function(window) {
                window.setTheme('theme1');
            });
        })
        .oldCapture('theme2', function(actions) {
            actions.executeJS(function(window) {
                window.setTheme('theme2');
            });
        });
    };
});

In this case Gemini probably will not fail, because in updated API you're going to use standard APIs, but if you will just exchange function without triggering old one, most likely you will have bad results.

One more point against decorator approach is that it is much less obvious than adding commands. Comparing gemini.addCommand('name', function() {}); and gemini.decorateSuite(function(builder) {}); more obvious is the first one, because it represents finite action of adding command.

I'm still against opening suite into public because it was designed to be private data chunk and I can't see any real reason why you need it in public. Writing tests depending on some internal state of testing framework is not seems to be a good idea.

In general, for your case I'd pick a way with custom commands instead of decorators.

I completely disagree that having a separate captureThemes function is better:

• More typing (minor concern)
• In my project, writing a test using capture instead of captureThemes would be incorrect
  • Everything should be tested in all themes
  • I'm not the only person writing tests. I want to point people at the Gemini docs and have the theming aspect be completely transparent to them
• If tomorrow we decide we only care about one theme, I don't want to have to change every single test in my project. I want to just remove the 1 line that loads the plugin.
• My plugin function will be nearly identical regardless of if the function is called capture or captureThemes. The only difference would be the first line that does var _oldCapture = this.capture, and using that function throughout. No difference otherwise.

I agree that addCommand is more intuitive, but at the cost of flexibility. The whole point of plugin systems is to allow flexibility. I think that addCommand would be a great feature that someone else might desire, but this is not what I would use.

Obviously the wrapped capture function would need to delegate to the original capture, I'm not suggesting otherwise. But as long as I don't break the API then why does Gemini care what my plugin does? I'm not asking for you to support my plugin.

I'm still against opening suite into public because it was designed to be private data chunk and I can't see any real reason why you need it in public

It isn't the data inside that I'm interested in, but having an object to store my own data on that gets inherited by nested suites. For instance, in my theming example, it would be nice to add a function called setThemes to override the default list of themes to be tested. This would work very similarly to setCaptureElements

suite.setUrl()
  .setCaptureElements()
  .setThemes()
  .capture()

EDIT: This last point is strictly a "nice to have" for me - I'm much more interested in the suite decoration than exposing the suite object. Just figure'd I'd explain the use case I had in mind.

@fenduru

More typing (minor concern)

This is not a concern at all. If you need less typing then you will name all your variables and functions using one letter.

I'm not the only person writing tests. I want to point people at the Gemini docs and have the theming aspect be completely transparent to them

But this will not work! If you replace original capture it will produce results different from what written in docs. So you will have to explain what's going on anyway. And this much less obvious than if someone will see captureThemes.

If tomorrow we decide we only care about one theme, I don't want to have to change every single test in my project. I want to just remove the 1 line that loads the plugin.

It's ok situation to change code and tests if requirements change. I can't see any problem here. Also if I understand it right, you will just have to refactor captureThemes to capture

Obviously the wrapped capture function would need to delegate to the original capture, I'm not suggesting otherwise. But as long as I don't break the API then why does Gemini care what my plugin does? I'm not asking for you to support my plugin.

In your case you're altering the existing API behavior, not creating new one. Nobody looks what inside plugins. If you will alter standard behavior and will not pass your knowledge to anybody in your team then when something will break people will blame gemini, not your plugin. And the debug will look like hell.

It isn't the data inside that I'm interested in, but having an object to store my own data on that gets inherited by nested suites.

You can just have variable in your test file and use it. Why this does not fit?

/cc @SevInf

@SwinX I'll update the PR to use the approach you prefer to get this moving.

You can just have variable in your test file and use it. Why this does not fit?

The themes to be tested are defined in the plugin's configuration, however in some rare cases we want to override this for particular components. I could make the signature of the captureThemes function optionally take the themes to test, but that is clunky since I want this setting for the entire suite, not a particular capture (in the same way that things like setUrl and setCaptureElements apply to the full suite/nested suites).

// I would like to write this:
gemini.suite('foo', function(suite) {
  suite
    .setUrl('...')
    .setCaptureElements('...')
    .setThemes(['themes', 'to', 'test'])
    .captureThemes('initial')
    .captureThemes('after something happened', function(actions) {
        actions.doSomething();
    });

  gemini.suite('nested inside of foo', function(suite) {
    suite
      .setCaptureElements('different from above')
      .captureThemes('initial'); // inherit the themes set in parent suite
 });

It seems that the only way the setUrl and setCaptureElements can support this nesting behavior is by having access to the suite object that gets inherited. I'd like my plugin to have the same capabilities to avoid this:

var themes = ['themes', 'to', 'test'];
gemini.suite('foo', function(suite) {
  suite
    .setUrl('...')
    .setCaptureElements('...')
    .captureThemes('initial', themes)
    .captureThemes('after something happened', function(actions) {
        actions.doSomething();
    }, themes);

  gemini.suite('nested inside of foo', function(suite) {
    suite
      .setCaptureElements('different from above')
      .captureThemes('initial', themes); // inherit the themes set in parent suite
 });