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

fixes #25363 - update jest and snapshots #6191

Merged
merged 2 commits into from Nov 18, 2018

Conversation

Projects
None yet
7 participants
@ohadlevy
Copy link
Member

ohadlevy commented Nov 1, 2018

No description provided.

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Nov 1, 2018

Issues: #25363

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 1, 2018

@sharvit @amirfefer @glekner @Laviro @boaz1337 any idea how to fix the failing tests? thanks

@amirfefer
Copy link
Member

amirfefer left a comment

Thanks @ohadlevy !
please notice that tests are failing

edit: sorry, i did not notice your comment 😳

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from 8fdaddd to 669445e Nov 1, 2018

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 1, 2018

@amirfefer thats what I was asking.. anyway, fixed the window.location failing parts, but @Laviro there is a test related to the autocomplete that fail, can you have a look please?

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from 669445e to b372bdb Nov 1, 2018

@ohadlevy

This comment has been minimized.

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 1, 2018

@@ -167,7 +162,7 @@ describe('updateTableTest', () => {
it('should reset page param to 1 after new search', () => {
const PerPage = $('#pagination-row-dropdown').text().trim();

window.location.href = 'http://localhost/?page=4';
window.history.pushState({}, 'Test Title', '/?page=4');

This comment has been minimized.

Copy link
@amirfefer

amirfefer Nov 1, 2018

Member

looks like it doesn't mocked, so should we use a teardown ?

@Laviro

This comment has been minimized.

Copy link
Contributor

Laviro commented Nov 1, 2018

@ohadlevy thanks for putting this up, I will look more into it later today or tomorrow.

@Laviro

This comment has been minimized.

Copy link
Contributor

Laviro commented Nov 4, 2018

after some time exploring, I found that the error:
TypeError: Cannot read property 'getInstance' of null

at first I thought that it is something connected with React fwd ref,
but it seems that the error is raised after trying to dispatch a keyboard event to the windiw/global:

const event = new KeyboardEvent('keypress', { keyCode: KEYCODES.FWD_SLASH });
global.dispatchEvent(event);

it worked before the last update but now it seem to break the test.

did anyone ever faced such a problem?
If it breaks this PR, you can xit the test and I will add a fix/ workaround to this problem later.

@Laviro

This comment has been minimized.

Copy link
Contributor

Laviro commented Nov 4, 2018

Can be fixed by #6201, been tested also with this updated Jest version.

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from b372bdb to e8deb12 Nov 5, 2018

@ohadlevy ohadlevy changed the title [WIP] fixes #25363 - update jest and snapshots fixes #25363 - update jest and snapshots Nov 5, 2018

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 5, 2018

@tbrisker mind having a look? test failures seems not related.

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 5, 2018

[test prprocessor]

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from e8deb12 to 4e58ef3 Nov 6, 2018

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Nov 6, 2018

There were the following issues with the commit message:

  • length of the first commit message line for 4e58ef3 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 6, 2018

Coverage Status

Coverage decreased (-14.4%) to 68.885% when pulling 7591944 on ohadlevy:jest-upgrade into e3eb262 on theforeman:develop.

@sharvit
Copy link
Contributor

sharvit left a comment

LGTM Thanks @ohadlevy

I'm not sure why coveralls decreased, locally it shows the same results.

When using with node-v6 it duble the time it takes to run the tests but I'm fine with it.

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from 027c2b2 to fae9b3b Nov 8, 2018

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 8, 2018

@sharvit it is indeed odd (coverage)

before the change:

=============================== Coverage summary ===============================
Statements   : 75.85% ( 1630/2149 )
Branches     : 64.84% ( 592/913 )
Functions    : 74.37% ( 470/632 )
Lines        : 83.25% ( 1526/1833 )
====================================

after the change

=============================== Coverage summary ===============================
Statements   : 69.32% ( 1640/2366 )
Branches     : 63.67% ( 594/933 )
Functions    : 70.48% ( 468/664 )
Lines        : 68.54% ( 1536/2241 )
================================================================================

note that there are more statements, I'm, not sure where this comes from - any idea?

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 8, 2018

@sharvit regarding being slow on node6, it could be a side effect of babel-jest, but I'm unable to confirm yet.

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from fae9b3b to 6d70de5 Nov 11, 2018

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 11, 2018

@sharvit any idea about the code coverage ?

@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Nov 12, 2018

@ohadlevy I ran over the differences between the reports and I can confidently say that the new coverage version can recognize more statements/branches/funcs/lines and it's great!

Some differences examples:

coverage-diff-1

coverage-diff-2

Let's merge it 👍

@ohadlevy ohadlevy force-pushed the ohadlevy:jest-upgrade branch from 6d70de5 to 7591944 Nov 18, 2018

@@ -38,9 +38,8 @@
"file-loader": "^0.9.0",
"highlight.js": "^9.12.0",
"identity-obj-proxy": "^3.0.0",
"jest-cli": "^20.0.0",
"jest-cli": "^23.6.0",
"jest-prop-type-error": "^1.1.0",

This comment has been minimized.

Copy link
@ohadlevy

ohadlevy Nov 18, 2018

Author Member

all packages listed in this file are not required for packaging, see https://github.com/theforeman/foreman-packaging/blob/rpm/develop/update-requirements#L8

@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Nov 18, 2018

@ohadlevy what is the status?

@ohadlevy

This comment has been minimized.

Copy link
Member Author

ohadlevy commented Nov 18, 2018

@sharvit I've rebased today, waiting for someone to merge ;) I'm still pounding about why the coverage changed...

@tbrisker
Copy link
Member

tbrisker left a comment

Thanks @ohadlevy and @sharvit !

@tbrisker tbrisker merged commit 5d9f585 into theforeman:develop Nov 18, 2018

7 of 8 checks passed

coverage/coveralls Coverage decreased (-14.4%) to 68.885%
Details
Hound No violations found. Woof!
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
foreman Build finished. 36738 tests run, 5 skipped, 0 failed.
Details
katello Build finished. 4083 tests run, 9 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished. No test results found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.