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

Bolt icons testing #1102

Merged
merged 12 commits into from Mar 15, 2019
Merged

Bolt icons testing #1102

merged 12 commits into from Mar 15, 2019

Conversation

adamszalapski
Copy link
Collaborator

Jira

http://vjira2:8080/browse/BDS-952

Summary

Write tests and updating configuration of jest tests

Details

(Explain the changes in enough detail fo reviewers to understand. Raise any questions for reviewers to consider.)

How to test

Run this code locally and check if all jest test are passing: npm run test:js

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@adamszalapski all tests passed, but I saw this error:

Error: ENOENT: no such file or directory, open '/Users/morsd/sites/bolt/packages/components/bolt-icons/src/icons/yeti.js'

It's looking for the icon in "src" not "__tests__". Is this expected?

image

@adamszalapski
Copy link
Collaborator Author

adamszalapski commented Mar 6, 2019

@adamszalapski all tests passed, but I saw this error:

Error: ENOENT: no such file or directory, open '/Users/morsd/sites/bolt/packages/components/bolt-icons/src/icons/yeti.js'

It's looking for the icon in "src" not "tests". Is this expected?

image

Thanks, @danielamorse is expected behavior. After compiling icons we remove the icon that was created only for these tests. Please check if you have this three task before all tests.
image
I will investigate what is going on. The biggest problem is when is running npm run test:js the error is occurring but if use npx jest packages/components/bolt-icons/__tests__/icons.js to run only this test, the error is gone. Maybe @sghoweri have some quick fix for that. If not i will try to fix this by myself.

@danielamorse
Copy link
Collaborator

@adamszalapski yep, those three tasks run.

It would be better not to get this error, as it makes me think that something is broken when I run the tests. If it's not too much work, can we try to fix?

I also noticed:

  • When I run npm run test:js, packages/components/bolt-icons/src/index.js gets modified with the line:
+ export * from './icons/yeti';
  • When I run npm start these two files are also modified/added:
m packages/components/bolt-icon/icon.schema.yml
+ packages/components/bolt-icons/src/icons/yeti.js

Ideally, git status would be clean after running start or test. Is that doable?

@adamszalapski
Copy link
Collaborator Author

@danielamorse and @sghoweri all issues that you guys raised should be fixed. Please try to check this PR one more time.

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@adamszalapski Tested again and now git is clean after running npm run test:js. I also reviewed the code updates. Nice work!

I have one remaining issue, but maybe it's a non-issue. When I first ran the tests they passed, but with errors on every icon, like this:

    ERROR in ./packages/components/bolt-icons/src/index.js
    Module not found: Error: Can't resolve './icons/youtube-solid' in '/Users/morsd/sites/bolt/packages/components/bolt-icons/src'
     @ ./packages/components/bolt-icons/src/index.js 116:0-38 116:0-38

Then I ran chmod 777 -R on the icons folder and it ran without any errors. Did you ever have this issue? Is there anything we can/should do about it? Approving the PR 👍 , just wanted to point out that issue.

@adamszalapski
Copy link
Collaborator Author

@danielamorse, in my opinion, is not related to chmod but with time when files are created. As far as i know, JS doesn't know when a file is created fully he knows only that command to create a file is finish. And index.js want to access files that not yet created. I will try to fix this but really i was thinking that i fix this because on my end this behavior is not occurring.

@danielamorse
Copy link
Collaborator

@adamszalapski ah, you are correct. I ran it again and the issue re-appeared.

@adamszalapski
Copy link
Collaborator Author

@adamszalapski ah, you are correct. I ran it again and the issue re-appeared.

Hi @danielamorse my assumptions were wrong. The problem is not that js don't know when files are fully saved but button tests use icons to and i recreate them after icons tests. Right now i move cleaning function to be run after all tests and i think that fix this issue. Please check on your end.

const config = await getConfig();

config.iconDir = config.iconDir.filter(
item => !item.includes('test/jest-test-svgs'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally these assets would live with the icon task or test for the icons themselves BUT I'm ok with this as is (we can always iterate) 👍

Copy link
Contributor

@sghoweri sghoweri left a comment

Choose a reason for hiding this comment

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

Looking great - awesome work @adamszalapski! 🎉

@sghoweri sghoweri mentioned this pull request Mar 15, 2019
@sghoweri sghoweri merged commit b0fd1f0 into master Mar 15, 2019
@sghoweri sghoweri deleted the feature/bolt-icons-testing branch March 15, 2019 21:12
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