Skip to content

fix test failures #1657

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

Closed
tomholub opened this issue Apr 17, 2019 · 8 comments
Closed

fix test failures #1657

tomholub opened this issue Apr 17, 2019 · 8 comments
Assignees

Comments

@tomholub
Copy link
Collaborator

I'm seeing consistent failures but it looks like a false alarm. I'll investigate it.

@tomholub tomholub added this to the 6.7.5 milestone Apr 17, 2019
@tomholub tomholub self-assigned this Apr 17, 2019
@tomholub
Copy link
Collaborator Author

not related but also affects tests

ERROR: /home/runner/flowcrypt-browser/extension/chrome/elements/pgp_block.ts[504, 123]: trailing whitespace
ERROR: /home/runner/flowcrypt-browser/extension/chrome/settings/fatal.ts[26, 130]: trailing whitespace
ERROR: /home/runner/flowcrypt-browser/extension/js/common/api/api.ts[329, 22]: (Anonymous function) must clarify return type. Implicit any is dangerous
ERROR: /home/runner/flowcrypt-browser/extension/js/common/api/api.ts[330, 14]: Unsafe use of expression of type 'any'.

@tomholub
Copy link
Collaborator Author

@michael-volynets
The following is dangerous:

    isInPrivateMode: (e: any) => {
      return e.message.startsWith('BrowserMsg() (no status text): -1 when GET-ing blob:moz-extension://');
    }

You may get various errors:

TypeError: a is null
TypeError: a.message is undefined

When you use any, you have to write code to make sure that it could really be anything. In javascript thrown object can be anything, not just an Error. That's why we use any, we don't know what it is.

But you cannot go from using any (don't know what it is) to writing code as if it was an Error. You have to test it.

One way to test it is:

    isInPrivateMode: (e: any) => {
      return e instanceof Error && e.message.startsWith('BrowserMsg() (no status text): -1 when GET-ing blob:moz-extension://');
    }

Now you fix the two errors mentioned above. You don't use any unsafely (first you checked it's an Error), and because TS now knows that e is Error, that means e.message is string, that means e.message.startsWith returns a boolean, so return type is now known.

@tomholub tomholub changed the title fix enterprise test failures fix test failures Apr 18, 2019
@tomholub
Copy link
Collaborator Author

@michael-volynets Assigning to you so that you can fix it and learn the right habits 👍

@tomholub
Copy link
Collaborator Author

Also, do you have tslint extension installed? It should alert you about at least some of these:

image

@tomholub
Copy link
Collaborator Author

You can also run npm run internal_tslint_dev_run to get this checked locally.

@tomholub
Copy link
Collaborator Author

I'll do this because I need to do a release soon.

tomholub pushed a commit that referenced this issue Apr 20, 2019
@tomholub
Copy link
Collaborator Author

It would be safer to use unknown instead of any everywhere, I'll make another issue.

@michael-volynets
Copy link
Contributor

Yes, I have tslint installed but I think I need to check if it's working correctly because I didn't see those alerts.
Okay, thanks

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

No branches or pull requests

2 participants