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

Adding unit tests for event listeners #6764

Open
3 of 17 tasks
RandomGamingDev opened this issue Jan 23, 2024 · 3 comments
Open
3 of 17 tasks

Adding unit tests for event listeners #6764

RandomGamingDev opened this issue Jan 23, 2024 · 3 comments

Comments

@RandomGamingDev
Copy link
Contributor

Increasing access

This would make it easier to test, and thus make sure that the event listeners are functioning properly on all platforms.
While event listeners may seem simple and not worth the effort to unit test, they can conflict or have obvious issues that are prevalent, but not easily found due to the differences between platforms that plague them.

For instance, the touchStarted() & touchEnded() functions, which both had doubling issues and issues with working with their corresponding mouse functions mousePressed() & mouseReleased() which wasn't detected for a long time and meant serious issues with mobile sketches.
The PRs are listed below:

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

Feature request details

Create a system for unit testing all of the event listeners and even combinations of related ones in order to make sure that they function properly.

@RandomGamingDev
Copy link
Contributor Author

I think it would be difficult to check for problems with event listeners using unit tests. BTW, there is a problem with our current logic, see here. #7195 #7260 my solution: DEMO

I think it's very exciting to create a new system, but shouldn't we first solve the problem at hand?

Just because something is difficult doesn't mean that it's unwarranted, and just because you can take measures to fix something, doesn't mean that preventative measures to stop them from happening in the first place are unjustified.

I think it's very exciting to finally fix and get past old issues, but shouldn't we first prevent them from happening?

@RandomGamingDev
Copy link
Contributor Author

RandomGamingDev commented Oct 12, 2024

I would just like to point out that I think it is may not good manners to reply with a message which form is similar to the one I have prepared("I think it's very exciting... and so on"). Honestly, it doesn't feel good. I guess that's the idea behind wanting to overlook people who are actually having trouble with bugs. I accept it. It's not me who is in trouble.

If you don't think that the format you used to message has bad manners you shouldn't have sent it in the first place. If you meant it in the sense that you didn't like it specifically when I tried to use it, I'd like to clarify that all I did was use the same reasoning process as you to show why I think implementing necessary measures to prevent future issues instead of pointing at current issues as a method of putting it down is necessary, and in a way that's in a way understandable to you. However, disliking that would still only point back to what you said being itself purposely offensive on your part, and this is while acknowledging my original assumption that you didn't mean it in an offensive or "bad manners" way.

Also, this issue isn't asking to overlook people having trouble with bugs. It's asking to create a system so that we can avoid people having troubles with bugs in the first place. It's you who said to avoid focusing on the system due to there being current issues which means that it's you asking to overlook future bugs that will trouble many, many people.

You say you want to take on the challenge even if it's difficult, but you really don't think it's that important, because you haven't worked on it for nine months, right?

  1. I never said that I wanted to take on the challenge despite its difficulty compared to other unit tests (although it still isn't that difficult, at least in terms of implementation).
  2. Not working on something for a while doesn't mean that you don't find it important. Just because I haven't went to all my nearest anti-global warming rallies doesn't mean that I don't think that global warming's important.
  3. Things take time, and changes to p5.js take support from p5.js stewards. You can't just expect someone to spend time that they may not have due to a life outside of the issue to dedicate their time to something that hasn't even been fully confirmed yet. That's just how open source works.

If you have a lot of things you want to do and are busy, it might be a good idea to ask someone else to do it for you.

The issue is currently open for just that.

I think you can do whatever you like.

To be honest, I don't think you believe that since you otherwise wouldn't have, as you've demonstrated, said

I think it's very exciting to create a new system, but shouldn't we first solve the problem at hand?

to purposely try and offend me (as suggested by your disliking of my usage of it) and say that the idea of preventative measures isn't warranted simply because there are currently still issues. Issues which, let me remind you, only got past because we didn't have a preventative system like what's being recommended in this issue in the first place.

@davepagurek
Copy link
Contributor

Hey everyone, I'm going to try to direct this discussion back to the problems at hand and how to address them.

Mouse + Touch expected behaviour

Part of the problem I think is that the browser handles mouse + touch events one way, and we have a slightly different set of expectations documented on the p5 site. Its true that by trying to fix one event handling bug, we inadvertently changed some of our other documented behaviour.

One option is to try to keep the simplified interface we currently document, which means a more complicated implementation. More tests would help prevent regressions as we work towards that.

Another option here is to try to be less different than the browser's default event handling. The fix here would then be to update our docs. The downside is that users have to be aware of more when trying to handle both mouse and touch. This would be a bit of a departure but is maybe something we can consider. Has anyone done touch + mouse handling in vanilla js and have more context on the drawbacks?

Priority

While adding tests isn't the top priority for maintainers right now, we are welcome to PRs that add them!

How to add tests for mouse and touch events

The key thing that potentially makes this difficult is that the behaviour of how browsers trigger touch events and mouse events is somewhat complex and is key to replicate correctly in a test, otherwise the test is misleading. Trying to do that manually by triggering fake events does seem to be a lot of work/research to be correct. However, it looks like we might be able to write a test where we can induce these events on a real live browser.

In the dev-2.0 branch where we're actively working on p5 2.0, we've switched to Vitest using webdriverio. It looks like webdriverio has some built in support for touch gestures? https://webdriver.io/docs/api/browser/touchAction/ This might make it easier to add testing for this, so if anyone is interested in adding unit tests, let me know and I can help get you started on the dev-2.0 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants