Update jasmine to 2.x #990
Conversation
|
I think this project has a guardian angel. Because every time I think “we don't have enough contributors to the core codebase,” someone comes in with a PR and says “I want to rewrite this extremely complex thing just because it's been bugging me.” I think you've bitten off quite a bit here, but if you can pull this off, I'd want to find a way to give you the Nobel Prize of Open-Source Contribution, an award that doesn't actually exist. One suggestion: every package, even built-in packages, can change their test runner independent of others. So it might be easier to introduce Jasmine 2.x as another option for test-runners; that way we'd be able to merge this PR earlier and migrate things over incrementally. We'd probably have to keep the Jasmine 1.x runner around anyway for community packages — unless we found a way to keep 100% backward-compatibility. |
kiskoza
force-pushed
the
update-jasmine
branch
3 times, most recently
from
3f5b584 to
e1463d1
Compare
|
I've been working on this one and have some updates: I've introduced jasmine2-test-runner and moved over pulsar's tests to run using it. Almost all tests are passing, there's only a few flaky specs that are failing here and there. I checked a few previous runs and they seem to be flaky already and sometimes the retry mechanism just skips them. I'm planning to re-introduce a retry workflow to make them pass again. Currently all the test fixes are in one gigantic commit, I'm planning to break it up into smaller ones and write an upgrade guide using / following the commits. Also, I broke the spec-helper file into several helper files. This way we can write some comments on why they are required, what are they doing and all of them holds one single responsibility. I've also kept jamine1-test-runner and made it the default for now if the package.json doesn't set anything else. I'm planning to convert at least one package to jasmine2 to make sure that it can pick up the new test runner. Also, I'd like to add a deprecation warning and set a date or milestone to remove it from the project. |
|
I did create an atom-jasmine2-test-runner and atom-jasmine3-test-runner for running tests in packages for atom a while ago if those can help. Those include updated packages for:
|
|
Oh... my...!!! Seriously, if it works, it will be AMAZING! I'm surprised! So, a few things, I found some tests that are using A question @kiskoza, how familiar are you with Jasmine, especially things like formatters and such? I'm asking basically because I've been wanting, for a while, to make tests run in a different way - it's a long WIP that lives on this branch: https://github.com/pulsar-edit/pulsar/tree/test-runner-based-on-mocha. Initially, I wanted to support Mocha, but then I though on doing some interface on Pulsar's side so that it would be a "bring your own library" thing - you implement these interfaces so that when you'll run a test, Pulsar will report failures and other stuff and you can keep whatever test library you want - be it Mocha, newer Jasmine versions, Jest, or even things on other languages. I'm not asking you to do that work, but I would love to discuss a way to implement these interfaces so I can do it; and then, we would not be limited to "whatever Jasmine version Pulsar supports", and migrations to newer versions could be done in smaller increments instead of "update all the tests". |
|
If this is truly an opt-in thing and doesn't break anything for community packages that rely upon our (ancient) test runner, that's amazing. It would even open the door to a gentle migration away from the old runner — we could change the default and allow community package maintainers to opt into the old runner if they really want. But I suspect that if they're actively maintaining their packages they'd be happy to make whatever (hopefully minimal) changes are needed to make their tests work in the new runner. (Now we just need a contributor to come by who's monomaniacally interested in fixing the small handful of issues we have with |
|
@savetheclocktower and one to optimize the text editor component (and rewrite to a UI framework that other people use), don't forget it 😆 |
|
Hi again! I finally got the time to clean up the latest bits on this PR and declare it ready 🚀 What did I achieve?
What's not in this PR?
What are the steps to reproduce this in the packages? As I mentioned before, I tried to return to the core jasmine framework as much as I can, so there's no helper methods to keep old codes and outdated soutions.
Let me know what do you think 😄 |
Explicitly needing to call But I don't want to bury the lead; this is still amazing work and I'm confident we can do what's needed to eliminate any further caveats.
These are classic markers of “decaffeinated” code — specs originally written in CoffeeScript and converted to JavaScript in automated fashion. Thanks for fixing them. |
|
Not sure if you answered this somewhere but why Jasmine v2? Jasmine v5 is out now |
I wanted to make a more granual uprade to start with. I had to figure out a lot of moving parts with the vendored codes and custom wrappers. When this is in, I'm going to continue my work to reach v5 sooner or later (could be one or more PR, depending on how hard it is to follow the changes) |
|
Hi @savetheclocktower and @mauricioszabo . I've pushed some codes to revert the Is there anything else I need to do to merge in this code change? Could you take a review? |
kiskoza
force-pushed
the
update-jasmine
branch
2 times, most recently
from
f573f07 to
50003de
Compare
|
OK, I'm taking a look at this PR tonight, the way I should've done a long time ago. Here's my experience so far:
So it seems as though we've got specs working great from the command line, but might need to do a bit more work to get them running graphically in the editor. @kiskoza, I should've at least tried this PR a few weeks ago to give you this feedback in a more timely fashion. I'm still gobsmacked that you were willing to work this hard on this PR and I'm determined not to let your great work go to waste. If you have questions, ask them here or find me on Discord; I'm more than happy to be a guinea pig for this PR. |
|
Good news is that I can comment out the reference to 
|
|
I threw myself against a wall yesterday evening trying to make a version of Here's what I came up with. It's roughly equivalent to what was there before, but I'll keep refining it. There are a few more changes I had to make to get it working; I'll note those as suggestions on this PR. |
| }; | ||
|
|
||
| const setSpecField = (name, value) => { | ||
| const specs = (new jasmine.JsApiReporter({})).specs(); |
There was a problem hiding this comment.
I was trying to diagnose why specType wasn't present on the specs I was dealing with, and traced it back to the fact that this line doesn't actually return any specs.
There's probably a way around this, but I also realized that I don't think specType ever gets a value other than user in any scenario; I haven't seen evidence of it in the core test suite, for instance. That might be a distinction that used to mean something but was eventually removed.
I also removed the code in my new version of AtomReporter that tries to make a nicer name for the suite by reading the package name from the spec directory, but we might want to put that back — in which case we'd want setSpecDirectory to keep working, so maybe we need to solve this problem after all.
Anyway, we can start at the top of the suite and crawl it to eventually build a flat list of specs — I do it in my new AtomReporter — but it isn't fun. I'll see if I can find a simpler way.
There was a problem hiding this comment.
I would just keep it for now and then we can do gradual improvements in follow-up PRs
Pro: * Less code to maintain & update * run-package-tests was unused Con: * Lost the formatting
|
Thanks for all the inputs. The non-headless runner worked like a charm with your fixes, I added them to a new commit |
|
Thanks! I'll revisit this when I get a chance; I got the new runner close to feature parity with the old one, but I should take another pass at it. There's another question I keep forgetting to ask: can community packages opt into the new test runner somehow? |
|
|
|
OK, after installing new modules and relaunching Pulsar, most of my problems go away… except that the runner does still crash. I don't think it's failing on a specific test, but it's hard to know for sure. 
|
|
I drilled into it; it's crashing on this line. I can step through a debugger and drill down into the internals to this line; stepping over it results in a crash. But it's native code, so I can't step into it to figure out why. I'll go back to |
|
OK, it's also failing on This is troublesome, but it seems like it's got nothing to do with this PR. Moving on. |
|
I've gotten reasonably far here. But the crashing issue I described above — though it's unrelated to this PR — throws a major wrench in my ability to assess this PR on my Mac computer. I might have to take a stab at proceeding on my rickety Windows machine. (I'm relieved to discover that the crashing doesn't seem to happen on a built version of the app on macOS; and if it were widespread, we'd surely catch it in CI. It's still vexing!) |
|
I merged |
|
Pretty late to be raising this, but rest assured I don't consider it a blocker for landing this change: I sense we should be building around the conventions described here — the ones that @UziTech relied on to build updated runners for Jasmine 2 and 3. I mention it because we've now got a lot of files sitting in When we land this, we'll have a vendored (and modified) version of Jasmine 1.3 living in the codebase alongside a Suppose we do something like this:
The indirection introduced by moving test runner code into a separate package solves the multiple-version issue; |
|
I've started fleshing out what I described in my last comment. It's simple enough in theory, since MockingFor instance: the Jasmine 1 test runner setup reaches outside of its own context quite often, pulling in stuff from There are also lots of places where the runner helpers require modules that expect to be the exact same copies of modules that are loaded by the codebase. This is how it can mock (e.g.) Does an external runner need to implement this stuff in the first place? If not, then the helper code that does all this stuff can stay in the Pulsar codebase. But if we want it to be an option for an external runner, then we could augment the That said, Fake clocksThe test suite mocks the clock by default before each test; methods like I would love it if we could reassess whether this truly makes sense. The mocking is necessarily imperfect and needs to be extended to cover things like The cause of this flakiness seems to be that And as explained in the section above, it’s even harder for an external runner to gain access to the things it wants to mock. It's also true that Jasmine 2 has its own mocked clock implementation. It might have the same pitfalls as the one that the Atom developers threw together, but it might still be worth looking into. But, honestly, it’s 2025 and we have |
|
The original idea behind making I always thought of Atom (and now Pulsar) as a great hacking tool so I don't believe there is any issue with making everything public as long as there is documentation on what can break between versions. |
Were there times that you chafed against these limitations? Any examples of things that you wanted to do with external test runners that couldn't be realized? Some of the mocks that the internal runner does are convenient, and good for Pulsar to do for its own tests, but don't seem essential to me, even for built-in package specs. And some of them could be done via other means — after all, you can get a reference to the |
|
The only push back I got was that they didn't wont some things public that they would need for the tests. The reasons had to do with breaking things without needing to release a breaking version. I think that was a valid concern when Atom was trying to be the biggest IDE but now I think with documentation saying something like "These things are public but could break between versions" it should be fine. |
|
I got an enthusiastic 👍 from @mauricioszabo on landing this, so I plan to do so immediately after we release 1.126 (which should be any day now). That way we'll have a few weeks to catch any possible regressions or newly flaky tests before we have to rely on the tests to do a release. |
| const values = this.values(); | ||
| while (!(next = values.next()).done) { | ||
| if (!other.has(next.value)) { return false; } | ||
| } |
There was a problem hiding this comment.
can be simplified
for (const value of this) {
if (!other.has(value)) { return false; }
}(and let next; on line 7 can be deleted too)
| let packageRoot; | ||
| if (FindParentDir == null) { | ||
| FindParentDir = require('find-parent-dir'); | ||
| FindParentDir ||= require('find-parent-dir'); |
There was a problem hiding this comment.
Note: I'm pretty sure ??= is supported, but of course this is probably better as a lint pr for later.
| window[key] = jasmineEnv[key]; | ||
| } | ||
|
|
||
| ['it', 'fit', 'xit'].forEach((key) => { |
There was a problem hiding this comment.
probably a good idea to comment something like
// Wrappers for tests: if the done parameter is not used, the wrapper calls done for you.
// The main reason tests need `done` is to use `jasmine.filterByPlatform(..., done)`
|
Landing this now rather than delay it any further. As discussed above, the next step is to modularize the runners so that they share less code and can be opted into in a standardized way by any package, but we'll get to that in due time. |
|
Hey! Thanks for merging this in, it was a huge PR with a lot of changes and discussions on them 🎉 I hope I can find some time to help with the next steps, we had a lot of open questions and follow-up tasks (and, of course all the packages that are using the jasmine1 runner right now) |
@kiskoza, I've got an assignment for you just in case you get to it first: package specs (the ones that use the jasmine1 runner) no longer seem to be able to use If you don't get to it before I do, no worries. |
|
You should be able to use https://github.com/UziTech/jasmine2-focused to add back fdescribe and fit to jasmine 2 |
5 participants
Hi. As mentioned before on #966 the current test suite is not in a very good shape. I've encountered most of the issues while I was creating other contributions, all my PRs (#785 and #954) had problems when I tried to cover the changes with specs.
What do we have already?
First of all, we have a running CI. I can only imagine the amount of work you had to put in to make it work 🥇 🙇
On the other hand, the tools behind the test suite got a bit outdated and messy over time. If you check the
package.jsonfile, you can see that we have three packages:jasmine-json,jasmine-reportersandjasmine-taggedas direct dependencies. Two of them seem to be abandoned already and the third one is pinned on a pretty old version. To make it more complicated,jasmine-taggeddepends onjasmine-focused(also abandoned) which depends on a direct commit ofjasmine-nodefrom github which seems to be v1.10.2 with some changes.Unfortunately, that's not all yet. As I tried to understand how the test suite works, I've found that we're only using a few code snippets from the jasmine package and most of the jasmine codes are coming from the vendor folder - we have a vendored jasmine v1.3.1. Also, the file starts with an ominous comment: "Modified line". Of course, if you make a proper diff with an official jasmine v1.3.1 release, there are more differences than the documented ones. We also have a vendored jasmine-jquery and some helper methods that are not related to jasmine, but all the methods defined here are available on window / global, so the vendored jasmine file was used as a place to put global helper methods. Uff.
The rest of the setup is easy to
understandbreak: we havespec/jasmine-test-runner.jsandspec/spec-helper.jsto set up the runtime and to add a lot of helper methods. Some of them are handling the test logging and the integration with the atom environment, but a huge chunk of it just tries to solve problems already solved by more recent jasmine versions, like re-running failed tests and handling async specs. If you feel like some codes are hard to read, then it's most likely an auto-generated javascript coming from the old coffescript files.What should we have?
Easy: a test suite without all the workarounds in the codebase. 🙃
To be more serious: The old jasmine version lacks a lot of quality of life features that a modern codebase / test suite should be able to rely on. I'm talking about retries without custom filtering, better spies and expectations on complex objects, supported asnyc tests, etc, etc. So in an ideal world, we should be able to use the latest-ish jasmine to write new tests or make the existing more reliable.
In this PR, I'm trying to replace all the mess with a working jasmine 2.x version. It won't solve all the issues immediately, but could be a good start.
In my commits
I would like to continue my work here - there's 70+ spec files in the spec folder and we haven't talked about package specs. As this could be a huge task, I can't promise anything. Also, I'd appreciate some feedback from the current state of the PR. Do you like the idea? Is there anything that you would do differently?