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

feat: add unit tests for the framework #35

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sniperadmin
Copy link

@sniperadmin sniperadmin commented Oct 9, 2021

This PR should do the following:

  • setup jest for the framework
  • Add some test cases as well as TDD cases for missing features
  • Add workflow for automated test coverage
  • Add Typescript support

@Kinrany
Copy link
Owner

Kinrany commented Oct 9, 2021

Thanks for the PR!

Could you add some motivation for it? I'm particularly interested in the code coverage integration and the specific tests you chose.

I want to err on the side of accepting all changes that improve something for the users and contributors, as long as the repo is small and easy to understand: vue-p5 is not a big or particularly well-maintained library, so I'd like to make sure anyone can fork it and change for their needs.

@sniperadmin
Copy link
Author

Sounds perfect!

I do agree to think and adapt what's necessary to achieve your target, so let me try adding more cases, maybe using some functional programming and setup reusable modules I guess

I do still have lots of random thoughts in my mind, hopefully I can organize them here ;)

src/p5.vue Outdated
* @param {object} sketch => sketch object
* @returns {object} of constants
*/
extractConstants: function (sketch) {
Copy link
Author

@sniperadmin sniperadmin Oct 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one works!
However, I think this approach needs to be checked as I may think it could be done better than this.

  • I thought to first generate the { key, value } set of the p5 constants first
  • Then, injecting them into the sketch just before emitting the sketch context

The cons of this approach:

  • I cannot test the method directly from the main component => gives undefined when I try to get the value in line 73
const savedKey = sketch[p5ConstName]

Workarounds

In the test file, I tested the component as if I am reusing it (re-usability). It returns the constants needed

@Kinrany
Copy link
Owner

Kinrany commented Oct 10, 2021

Let's take a step back. In what way would these changes improve the library for you?

For example, as a user I'd like to avoid typos in event names. So I might add TypeScript support so that all typos are caught at design time. This makes me as a contributor want to change how the build tooling is set up. So I might add a unit test to be confident that the build output still works in the browser.

To clarify, I'm not suggesting these specific changes. I want to know what part of the user/contributor experience will be improved by the changes you're suggesting.

@sniperadmin
Copy link
Author

I actually do love your feedback because it helps me organizing my thoughts a bit
I do agree of course on what's been stated in your comment.

let me mindmap my thoughts one more time and take those points into concideration

This re-implements the previous commits with a better approach
@sniperadmin
Copy link
Author

sniperadmin commented Oct 13, 2021

Type-script has nailed it 🎉

New advantages:

  • I discovered that the old way of re-exporting p5 doesn't work, so I injected it as a separate payload, we can use it in the sketch event (for now) like this:
sketch(sk: any, p5: any): void {
  this.p5 = new p5()
  this.sk = sk
}

The test file explains the approach

  • Based on the previous adv. I have reverted the old way of retrieving constants as it is not useful anymore. Now we can call the constants, Vectors and many other p5 methods and classes easily like this:
sketch(sk: any, p5: any): void {
  const p5 = new p5()
  const pi = p5.PI  // 3.141592653589793
}

The Pros and Cons.

Pros:

  • Getting p5 as a separate arg in the sketch() method will let us use third party libs like p5.sound()
  • Now we can use almost anything in p5, but need to complete test cases for that one

Cons

  • We cannot say VueP5.something.. The user can use the original p5 itself as an argument in the sketch event

Next up:

  • If this approach is granted, we can globalize the p5 across all the remaining events, so that we can use it in them:
    e.g.
<template>
   <vue-p5 v-on="{setup, draw, keypressed}"></vue-p5>
</template>

<script>
export default {
  methods: {
    draw(sk, p5) {
      // ...
    },
    keypressed(sk, p5) {
      // ...
    }
  }
}
</script>

@Kinrany
Copy link
Owner

Kinrany commented Oct 13, 2021

We cannot say VueP5.something.. The user can use the original p5 itself as an argument in the sketch event

This is a big cons: the p5 reexport is there specifically to expose p5 outside of a sketch context. Inside a sketch context the sketch object is a replacement for the p5 object.

@sniperadmin
Copy link
Author

Good Call, I tried actually to use the previous reexport to call consts or something like p5.Vector, but got undefined, any thoughts around this?

if is it a buggy behavior with the reexport itself, how should we test that?

@Kinrany
Copy link
Owner

Kinrany commented Oct 13, 2021

I never used it in any of the examples, so it may be broken indeed!

If it is, this is probably a build tooling issue. The test would have to use build output as a dependency and run an equivalent of

const VueP5 = require('vue-p5');
console.assert(VueP5);
console.assert(VueP5.p5);
console.assert(VueP5.p5.abs);

@sniperadmin
Copy link
Author

Here is an issue if I tried to use the reexported p5:

sketch(sk: any): void {
  const p5 = new sk.p5()
}
TypeError: sk.p5 is not a constructor

      56 |       methods: {
      57 |         sketch(sk: any): void {
    > 58 |           const p5 = new sk.p5()
         |                     ^
      59 |         }
      60 |       }
      61 |     })

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

Successfully merging this pull request may close these issues.

2 participants