Re-add check #229
Re-add check #229
Conversation
| ) | ||
| ).to.equal(1); | ||
| }); | ||
| // it('evaluates object literal after other statements as block', async() => { |
There was a problem hiding this comment.
this is just a difference we have with the usual node repl or chrome devtools:
in node:
> ;{x: 1}
1
>
in browser-repl:
;{x: 1}
undefined
I don't know what is your take on this, probably can be just ignored
There was a problem hiding this comment.
Oh interesting. It's treating {} as a block scope and not as an object...which would explain why if you just put in {x: 1} then it will give you a syntax error. We could make a ticket for it but I don't think it should be a blocker.
There was a problem hiding this comment.
Here's the ticket: https://jira.mongodb.org/browse/MONGOSH-256
I will keep the tests as skip so they will warn as a reminder TODO.
| ) | ||
| ).to.equal(1); | ||
| }); | ||
| // it('can redeclare a top level function as function', async() => { |
There was a problem hiding this comment.
this may really be an issue, again a corner case but this looks more realistic:
function f() { return 1; }
[Function: f]
function f() { return 2; }
[Function: f]
f()
1
Expected:
function f() { return 1; }
[Function: f]
function f() { return 2; }
[Function: f]
f()
2
There was a problem hiding this comment.
I think it's worth a ticket, but will also be closed when we do the unify eval epic so I'm happy to keep this as a skip so we're reminded that there's some TODO there
| // it('should not wrap multiple statements', () => { | ||
| // // TODO: for some reason this is the default behaviour in node repl and devtools, | ||
| // // as is now it wraps everything, which breaks the code. | ||
| // expect(wrapObjectLiteral('{x: 2}; {x: 2}')).to.equal('{x: 2}; {x: 2}'); |
There was a problem hiding this comment.
super corner case, again we can keep it or drop it, maybe give it a look
There was a problem hiding this comment.
I think this can go under the ticket https://jira.mongodb.org/browse/MONGOSH-256
| // context('when cli args have disableImplicitSessions', () => { | ||
| // const cliOptions: CliOptions = { disableImplicitSessions: true }; | ||
| // | ||
| // it('maps to explicitlyIgnoreSession', () => { |
There was a problem hiding this comment.
have no clue about this one
| }); | ||
| }); | ||
| }); | ||
| // context('when running against a database', () => { |
There was a problem hiding this comment.
This seems to pass so will keep it for now, unless it breaks later on
There was a problem hiding this comment.
Nope, fails on evergreen, so will remove it
| // ).to.equal(2); | ||
| // }); | ||
| // | ||
| // it('can redeclare a top level function as var', async() => { |
There was a problem hiding this comment.
this is also kinda common and unexpected
There was a problem hiding this comment.
Will keep it as a skip then and include it in the ticket
|
I think it was originally disabled because it was trying to lint the cli-repl/dist/mongosh.js file and it was causing a buffer overflow with no error. Once we ignore that we don't get any more random failures I think. |
We were also missing linting in a few packages, including the service providers so I had the distinct displeasure of linting all those packages.
I commented out skipped tests because they were warning, up to us if we want a warning-free master or not. As is we have 0 warnings or errors. I'm okay with warnings for things that are TODOs, but we should probably discuss.
Also there's a bug with typescript linting where interfaces and types that are used get registered as undefined, so I changed most interface definitions to
export default interface xxxwhich is a workaround.