-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Fix-karma #1157
Fix-karma #1157
Conversation
Make sure chrome and firefox are on your path. If you would not like to edit your path, make a simple script that sets CHROME_BIN and FIREFOX_BIN
Could you try to remove all the browserify modules? I think there is a webpack-karma that might be better. Working on fixing CI right now for PRs |
Sounds good. |
Here is where to turn the tests back on https://github.com/jimp-dev/jimp/blob/master/.github/workflows/build.yml#L69 |
@hipstersmoothie are we going to change browser testing framework in the TS release/next branch? |
I think keeping the ability to run the tests in the browser is a good idea. I think karma is kinda old and we could replace it with this and playwright and get something better Since I want to test in browser mocha will probably stay but I'm working on switching the assertion library from should to expect. |
The TS refactor shouldn't effect any of this |
Well expect doesn't work in browser </3 |
yes but point is why not change testing framework when changing code as well |
@hipstersmoothie Moving to webpack is causing issues with |
Id try the newer tool before any further getting karma working. If it makes it simpler you can also assume for the browser tests that jimp has already been bundles and exists in packages/jimp/browser/lib |
@hipstersmoothie As some pointed out, we still have to maintain it for some time. We should still use Karma for the JS version of Jimp |
@hipstersmoothie With that being said, even if we go to playwright I belive I would still have to debug this |
Fixes #1129 |
Issue found. esmodules don't have a prototype Resorting to .default should be the fix Edit 2: Preserving legacy behavior with es modules |
@nopeless I may have a PR that makes this easier |
If you are talking about #1163, it doesn't fix the webpack issue. The conversion from CJS to ESM breaks tests as well |
What/how are you web packing? The library itself already bundles fine we should be able to use that bundle to run tests with in a browser. Could you file an issue for this and propose what you think it should be? |
import { Jimp, mkJGD } from "@jimp/test-utils";
describe("Compare image difference", () => {
let imgs = [
Jimp.read(mkJGD("2222", "4444", "6666", "8888")), @hipstersmoothie It seems that the Jimp here is the esmodule Jimp itself not Jimp class. That is what's causing the issue |
Ahhh I see |
I currently have a hack where on |
Ok so if you're trying to get karma to still work I think we should stop that The steps I see for running the tests in browser:
|
I am almost certain that this isn't a Karma issue. (Unless you are talking about the esoteric loading system that is in place for karma at the moment. That is certainly an issue that this PR will fix) If, however, configuring esbuild and an alternative testing system implies removing the current one, I am definitely up for that. For now, I don't think we need more changes other than using webpack. |
Yeah I def wanna remove karma entirely. mocha + expect are here to stay but karma I want gone |
Well, we can do that in another PR. I want Karma at least running so it can run against other fixes while the test migration is in progress |
Sounds like a good plan! |
Apparantly the newest build updated yarn.lock for packages as well since I had updated test-utils With that being said, why is test-utils in src in the first place |
@nopeless you can update the lock file just by running |
Organizing packages like this is pretty standard for JS monorepos |
The lock file is already updated. I just found it weird that tests loaders are coupled with the versioning. I understand why tests are part of the versions but the loaders should be minimal and not versioned; A gotcha moment for me |
@hipstersmoothie I see wht you mean |
whoops this closed cause I renamed master |
Karma has been fixed