Users/madhurig/rubytestswithrunner #7369
Conversation
Tasks/UseRubyVersionV0/Tests/L0.ts
Outdated
| function reload(): typeof useRubyVersion { | ||
| return require('../userubyversion'); | ||
| } | ||
| import fs = require('fs'); |
There was a problem hiding this comment.
Minor, looks to be an unused import.
Tasks/UseRubyVersionV0/Tests/L0.ts
Outdated
| versionSpec: '= 2.5', | ||
| addToPath: false | ||
| }; | ||
| it('finds version in cache in Linux', (done: MochaDone) => { |
There was a problem hiding this comment.
We need a linter rule for this ;)
| process.env['AGENT_VERSION'] = '2.116.0'; | ||
|
|
||
| // provide answers for task mock | ||
| let a: ma.TaskLibAnswers = <ma.TaskLibAnswers>{ |
There was a problem hiding this comment.
nit: can we delete this if it isn't needed, or at least remove the type assertion (<ma.TaskLibAnswers>)?
| fs.existsSync = (s) => { | ||
| return true; | ||
| } | ||
| tr.registerMock('fs', fs); |
There was a problem hiding this comment.
I feel like rebinding actual functions on the fs module could cause a confusing bug. Also, if you use a brand-new mock object like you did with tool-lib above, it will tell you right away that you forgot to mock something, where using the actual module will sneak through silently.
Tasks/UseRubyVersionV0/Tests/L0.ts
Outdated
| } | ||
| import fs = require('fs'); | ||
| import assert = require('assert'); | ||
| import path = require('path'); |
There was a problem hiding this comment.
The import ... require syntax is made for compat with some other JS module loaders, but the ES6-standard import * as path from 'path'; gets treated more strictly and is usually what you want.
For example, TypeScript will treat all of the properties on the module imported with import as as read-only, where with require it will let you reassign them.
There was a problem hiding this comment.
Will update, we have both styles in the same file which is not great. Is there a order of imports convention in TS?
There was a problem hiding this comment.
From looking at some other tasks it looks like we do Node modules first, then task lib, then 3rd-party modules.
| symlinkSync: () => { }, | ||
| unlinkSync: () => { }, | ||
| existsSync: () => { return true; }, | ||
| statSync: () => fs.statSync, |
There was a problem hiding this comment.
I think statSync, writeFileSync, readFileSync must not be used. The mocks here wouldn't work as expected if called -- they're functions that are returning a function object :)
There was a problem hiding this comment.
They need to be mocked, but should fall through.
Convert UseRubyVersion tests to use the MockTestRunner. This allows running each test out of proc simulating how tasks are run.