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

#237: Removes Jasmine + Karma. Introduces Jest + JSDOM + Testing Libr… #242

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

bukhtiyarov-a-v
Copy link

#237: Removes Jasmine + Karma. Introduces Jest + JSDOM + Testing Library. Introduces simple changes into tests that are required to make tests work with new matchers.

…ting Library. Introduces simple changes into tests that are required to make tests work with new matchers.
@bukhtiyarov-a-v
Copy link
Author

@wbotelhos please check this out :)

beforeEach(function () {
this.target = Helper.create('#target');
beforeEach(() => {
testContext.target = Helper.create('#target');
Copy link
Owner

Choose a reason for hiding this comment

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

💅 To avoid creating an extra variable and reset, we could just select this element on the expectation.


done();
}, 100);

jest.advanceTimersByTime(100)
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need it? The setTimeout was just because sometimes the target was not filled yet.

@@ -291,7 +299,7 @@ describe('#precision', function () {
Helper.trigger(cancel, 'mouseover');

// then
expect(this.target).toHaveHtml(raty.opt.cancelHint);
expect(testContext.target[0]).toContainHTML(raty.opt.cancelHint);
Copy link
Owner

Choose a reason for hiding this comment

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

Could we use the simplest selector possible like innerHTML? So we depend less on the framework.

// given
var raty = new Raty(document.querySelector('#element'), { cancelButton: true, space: true });

// when
raty.init();

// then
expect(raty.element.innerText.length).toEqual(5);
expect(raty.element.textContent.length).toEqual(5);
Copy link
Owner

Choose a reason for hiding this comment

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

Since it's a normal element, should we use textContent?

@@ -713,7 +713,7 @@ class Raty {
_setTitle(score, evt) {
if (score) {
const integer = parseInt(Math.ceil(score), 10);
const star = this.stars[integer - 1];
const star = this.stars.item(integer - 1);
Copy link
Owner

Choose a reason for hiding this comment

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

Is it to avoid out of range value? I think we have an issue with mouseover when it's close to the cancel button where the value comes as negative, but I think it was already addressed.. don't know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Coz this.stars is not an array, but iterable.

@@ -1,18 +1,26 @@
function context(description, spec) {
const $ = require('jquery');
Copy link
Owner

Choose a reason for hiding this comment

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

Could we try to drop the jQuery? Or it would be in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be in another PR

@wbotelhos wbotelhos merged commit 8b73703 into wbotelhos:main Oct 18, 2022
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.

None yet

3 participants