Add "real" example test

[wilhelmklopp]

The test currently included as part of the template is not super helpful for folks getting started with testing their probot apps. The intention of this PR is to make it a little bit easier for them by providing a working example that they can use as a starting point and either copy, modify, or extend.

The example is basically the same as the one presented in the docs, with some minor tweaks.

This PR also adds the "Hello World" example from the probot/probot README to index.js which should make it even easier for folks to get started.

[JasonEtco]

Can this be let app, github? Or do you prefer it like this (I prefer the first, but I won't die on this hill)

[wilhelmklopp]

done! We should also update this in the docs and anywhere else it's used for consistency

[hiimbex]

I don't think we should use plugin here. I'd prefer to rename this to app and rename app to probot.

[tcbyrd]

🤔 Hm, but the class is called Application, so that would look like this:

const { Application } = require('probot')
const probot = new Application()
const app = require('..')
...
probot.load(app)

We do tend to call these plugins elsewhere (like the 3 default plugins for stats, sentry, and the default website). But I see the confusion since we do module.exports = app => {...}. I'm inclined to leave it as plugin and maybe explain that reasoning a little better in the docs?

[hiimbex]

I'm having flashbacks to probot/probot#210 but I think we should be very certain about our language choices before merging this, since everyone will use that language once we do it.

I also don't think that code snippet is all that horrible sounding, but I'm open to other thoughts.

[JasonEtco]

I agree with @tcbyrd that that code snippet is confusing - I'd expect app to be an instance of Application, just based on the naming.

However, I agree that plugin is 👎, especially because of that issue @hiimbex was kind enough to link to. Some other ideas:

• applicationFunction (which is actually the TS name)
• appFunction
• myApp
• myProbotApp
• Fill this with a camel-cased version of their app name, as part of the create-probot-app script

[hiimbex]

I think all of these are good options, but the idea of mimicking the actual TS name sounds the best to me. But I don't have strong opinion on this except to not use plugin

[tcbyrd]

I think what we're struggling with here is the default export of the template doesn't have an app name. What we should probably be encouraging is using the name of the app instead of "plugin".

So for TODO:

const { Application } = require('probot')
const app = new Application()
const todo = require('..')
app.load(todo)

or WIP:

const { Application } = require('probot')
const app = new Application()
const wip = require('..')
app.load(wip)

I like using applicationFunction because it's the best description of the general term, but maybe we could leave a comment that it could be whatever your name your app?

[JasonEtco]

@tcbyrd, thanks for finding those examples. Given that we name the variable after our apps, it sounds like this idea would be the best, but also the most effort to implement:

Fill this with a camel-cased version of their app name, as part of the create-probot-app script

[JasonEtco]

Et voilà: probot/create-probot-app#30

[wilhelmklopp]

One concern of this approach is that it kind of breaks if folks name their probot app just "app" or any other reserved word, but not sure if that is a huuuge problem.

Another option is to use myProbotApp instead of plugin?

[bkeepers]

👏 yesss!

[wilhelmklopp]

2 questions I have before we merge this:

1. Since the update to this template requires create-probot-app to pass a new variable, does that mean create-probot-app will break for all users stuck on an old version?

2. My previous comment on the now collapsed thread:

One concern of this approach is that it kind of breaks if folks name their probot app just "app" or any other reserved word, but not sure if that is a huuuge problem.
Another option is to use myProbotApp instead of plugin?

I'm starting to learn more towards sticking with a generic name 🤔

[hiimbex]

Since the update to this template requires create-probot-app to pass a new variable, does that mean create-probot-app will break for all users stuck on an old version?

I'm not sure I follow. name already exists and is dropped into the readme.

One concern of this approach is that it kind of breaks if folks name their probot app just "app" or any other reserved word, but not sure if that is a huuuge problem.

I'm not convinced this is a common enough issue to deal with, but willing to hear other opinions on this.

[wilhelmklopp]

I'm not sure I follow. name already exists and is dropped into the readme.

Instead of name it would be camelCaseAppName: probot/create-probot-app#30

[wilhelmklopp]

I'm not convinced this is a common enough issue to deal with, but willing to hear other opinions on this.

It's funny because this actually happened to me while testing this new template. I created a probot app called "test" and of course that broke things because it's a reserved word in Jest 🤣. Took me a while to realise what was going on 😅

[JamesMGreene]

Thoughts on switching the test calls over to it calls? Obviously the test names/titles would need to be slightly modified as well if we do that.

This would also align with the example from the Probot testing docs: https://probot.github.io/docs/testing/

[wilhelmklopp]

Thoughts on switching the test calls over to it calls? Obviously the test names/titles would need to be slightly modified as well if we do that.

I think I personally prefer test, because it seems to to be the default in Jest? But don't feel very strongly about it.

This would also align with the example from the Probot testing docs: https://probot.github.io/docs/testing/

It feels like after this PR is merged, we'll update that code snipped accordingly to bring the two in line

Probot

Testing

GitHub Apps to automate and improve your workflow

[JamesMGreene]

I think I personally prefer test, because it seems to to be the default in Jest? But don't feel very strongly about it.

Hmm, yeah, that does seem to be the case. I'm so used to unit test frameworks with describe + it for BDD interfaces, and suite/module + test for TDD interfaces. Jest apparently went rogue and decided on describe (and only describe) + test (aliased to it). Odd to me but I can roll with it.

It feels like after this PR is merged, we'll update that code snipped accordingly to bring the two in line

Yes, that would definitely be ideal. 👍