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.

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

Unable to stub static method on ES6 classes #164

Closed
LandonSchropp opened this issue Dec 13, 2016 · 4 comments
Closed

Unable to stub static method on ES6 classes #164

LandonSchropp opened this issue Dec 13, 2016 · 4 comments

Comments

@LandonSchropp
Copy link

I'm loving the changes in #158, but I do seem to be missing the ability to stub out a static method on an ES6 class. Ideally, I'd be able to create a mock for this:

module.exports = class Hello {
  static hello() {
    return "hello";
  }
}

Like this:

helloMock = td.replace("./hello");
td.when(helloMock.hello()).thenReturn("goodbye");

I realize that might conflict with the instance methods, and I don't have a good suggestion as to how to rectify that situation other than to merge the two sets of methods into a single object and to override duplicates.

@LandonSchropp LandonSchropp changed the title Unable to stub static method on ES6 class Unable to stub static method on ES6 classes Dec 13, 2016
@searls
Copy link
Member

searls commented Dec 13, 2016

Ok, two answers, first the technically correct one

The technically correct answer

Because the double returned by td.replace() quacks like an instance of the instantiable, this won't be possible with the 1.x API. Instead, if you need a fake of the static API, I recommend requiring it and calling replace on that:

const MyClass = require('./my-class').default 
td.replace(MyClass, 'staticMethod')

var subject = require('./my-subject')
// … test stuff

td.reset()

Also, I suspect if you called td.replace on my-class first and then required it in this manner (so that both the class and its instances are faked, you'd accomplish what you want, but I haven't tested it.

The opinionated answer

td.js will never go out of its way to make it really easy to design dependencies that would require this as a feature. If a design calls for a lot of pure functions that are namespaced in modules, that's great. If a design calls for instantiable classes, that's great too. But 10 years of prior art in isolated TDD in C# and Java (google "mock static method" for either) suggest that static methods on classes are an awkward design decision and almost never what an isolated TDD workflow would naturally call for. I don't want to relitigate "static methods are {good,bad,ugly}!" here, however.

I'll try to put it another way:

In a language with first-class addressable functions like JavaScript, I see even less reason to fret about the ability to assign static methods on classes. There are cases where static methods make great sense (for example, an instance method for a singleton class), but what I've learned from practicing GOOS/London-Style/Discovery Testing is that when it comes to classes, small, easy-to-construct, boring classes are almost always preferable to intelligently-designed ones that adhere to patterns which would require static methods.

When I'm practicing TDD, my classes almost always look like this (using ruby as pseudo-code):

class PerformsAction
  def initialize(data_about_action_like_system_config_or_something) 
    @config = data_about_action_like_system_config_or_something
  end 

  def perform(unit_of_work)
    # do the work
    return some_result
  end
end

Since I don't plan on going out of my way to figure out support for this, I'm going to close this. However, I do think you would be covered by the enhancement described in #54.

@searls
Copy link
Member

searls commented Dec 14, 2016

I'm reopening this because I'm having a bit of a change of heart that the current API is too confusing now that people are thinking of instantiable functions even more as "classes" as they used to. Read #159 for a longer discussion on this.

I think keeping the return value symmetrical is just going to be much more obvious and require fewer workarounds, but I'm struggling to think of how to do this without making a breaking change or adding a litany of .prototype references to every test that doesn't care about the constructor and just wants to get at the double functions.

@searls
Copy link
Member

searls commented Dec 14, 2016

We're going to make this work by way of #166. Will close this to track it there.

@LandonSchropp
Copy link
Author

Thanks for the replies @searls. If it helps, the real-world use case for using a static method in our app is to simulate an ORM-style object that wraps a graph service. So our class looks something like this (a simplified example):

class User {
  
  constructor(attributes) {
    _.extend(this, _.pick(attributes, ["firstName", "lastName", "email"];
  }

  get name() {
    return `${ this.firstName } ${ this.lastName }`;
  }

  update(attributes) {
    return graphClient.command("updateUser", attributes).then(...);
  }

  static findByEmail(email) {
    return graphClient.query("user", { email }).then((attributes) => new User(attributes));
  }

  static all() {
    return graphClient.query("users").then((userData) => userData.map((attributes) => new User(attributes));
  }
}

In our case, I don't think the static methods on the class are being used in the way you're describing above (i.e. they're not simple utility methods). Sure, we could create separate functions to run queries on our model objects, but it's very simple and convenient to have them present on the class itself. It also makes sense to encapsulate all of the query logic within the class itself.

I'm not trying to suggest that any of your points are wrong or anything like that. I just thought you might like to see a more realistic example.

willkoehler added a commit to willkoehler/geohashing-exercise that referenced this issue Mar 13, 2017
Making Randomizer a constructor function doesn’t do anything for us. All we really need is a single function with several helper functions. Use the simplest design that solves the problem.

This became apparent when we tried to create a test double for Randomizer and ran into this testdouble/testdouble.js#164 (comment)
willkoehler added a commit to willkoehler/geohashing-exercise that referenced this issue Mar 13, 2017
Making Randomizer a class doesn’t do anything for us. All we really need is a single exported function (randomPair) with several helper functions. Use the simplest design that solves the problem.

This became apparent when we tried to create a test double for Randomizer and ran into this testdouble/testdouble.js#164 (comment)
willkoehler added a commit to willkoehler/geohashing-exercise that referenced this issue Mar 13, 2017
Making Randomizer a class doesn’t do anything for us. All we really need is a single exported function (randomPair) with several helper functions. Use the simplest design that solves the problem.

This became apparent when we tried to create a test double for Randomizer and ran into this testdouble/testdouble.js#164 (comment) (see "The opinionated answer")
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

No branches or pull requests

2 participants