CM-807: Upgrading build dependencies #115
Conversation
…SUE-upgrade-build
…SUE-upgrade-build
SuperIzya
force-pushed
the
NOISSUE-upgrade-build
branch
from
1edb436
to
48e72ac
Compare
SuperIzya
changed the title
SuperIzya
force-pushed
the
NOISSUE-upgrade-build
branch
from
20c7c5d
to
5716e54
Compare
| command: | | ||
| curl -Os https://uploader.codecov.io/latest/linux/codecov | ||
| chmod +x codecov | ||
| ./codecov -t ${CODECOV_TOKEN} |
There was a problem hiding this comment.
I think the value of CODECOV_TOKEN needs to be set in circleci env vars, or does it read it from somewhere else?
There was a problem hiding this comment.
You can get the CODECOV_TOKEN in circleci env variables here: https://app.codecov.io/gh/LiveIntent/live-connect/settings
There was a problem hiding this comment.
Thanks. But before, I have to make sure that the tests are generating proper data for codecov (it's a matter of running the tests)
| import { fileURLToPath } from 'url' | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)) | ||
| const packageJson = JSON.parse(fs.readFileSync(path.resolve(__dirname, '..', '..', 'package.json'), { encoding: 'utf-8' })) |
| window = win | ||
|
|
||
| expect(getPage(fw)).to.be.eql('https://liveintent.com/about?key=value') | ||
| // eslint-disable-next-line no-global-assign | ||
| window = oldWindow |
There was a problem hiding this comment.
Should a try with resources that guarantees that the window is restored to its old state be used here? Something like this: https://github.com/LiveIntent/live-connect/pull/116/files#diff-b1c4e1833e06200917cbcbfcd57a775db133813709cf27170f2325bc45a4bcf9?
There was a problem hiding this comment.
This is a test. If it fails, then the code is wrong. I actually want it to fail in most spectacular way. That's why I don't want to put try-finally in this particular case.
There was a problem hiding this comment.
Are the changes to window visible outside of that test -e.g. if the expectation is not fulfilled and the test fails? If yes, I would rather prefer to keep it local to that test.
There was a problem hiding this comment.
Again, if the expect fails and the code never gets to window = oldWindow, which will lead to other tests failing - I don't really mind. IMHO, the tests should work in optimistic scenario and fail in any other case.
There was a problem hiding this comment.
I think tests should be isolated so that it is easy to understand what is broken. A test having effect on other tests - even if it is only in case of failure - is usually not what you want.
There was a problem hiding this comment.
Actually, it does not affect other tests.
Tested with failing expectation expect(getPage(fw)).to.be.eql('https://liveintent.com/about1?key=value') - only 1 failed test
There was a problem hiding this comment.
My point is that if it is used by other tests, it should be restored to its original state before the test ends. Tests should have no effect on each other and should not run using each other's state. I do not want to discuss it for too long. I leave it up to you if you change it or not.
# Conflicts: # .circleci/config.yml # package-lock.json # package.json # test/unit/events/error-pixel.spec.ts # test/unit/manager/decisions.spec.ts
# Conflicts: # README.md # package-lock.json
All npm dependencies upgraded to remove outdated/deprecated/vulnerable ones.
Author Todo List:
Ready For Reviewstatus