test: tests for createTransactionByType and createTransactionFromLog #4587
Conversation
…added Signed-off-by: Mariusz Jasuwienas <[email protected]>
arianejasuwienas
added
enhancement
|
|
||
| // TransactionFactory is a factory class that creates a Transaction object based on the type of transaction. | ||
| export class TransactionFactory { | ||
| public static createTransactionByType(type: 2, fields: any): Transaction1559; |
There was a problem hiding this comment.
i wonder why didn't this result in a failure, if we were passing other types
There was a problem hiding this comment.
@konstantinabl I believe the root cause lies in this line:
We used any as the type for the contract result, so even though this field could potentially contain any number (which we later pass to createTransactionByType), type checking is effectively disabled there.
(other call to createTransactionByType uses the number 2 explicitly)
| accessList: ['should be ignored'], | ||
| }); | ||
|
|
||
| expect(tx).to.not.equal(null); |
There was a problem hiding this comment.
I see a lot of repetitive code asserting tx.type tx.from tx.accessList, can we create some sort of function? Do you think it makes sense?
There was a problem hiding this comment.
I think these kinds of repetitions aren’t too problematic in tests.I’ve only moved the whole-object comparison into a dedicated function, since that made more sense. And even naming it was easy (what would be the best name for those 3 fields comparison: expectTxTypeFromAndAccessList or expectBasicFields?)
That said, I’m not insisting on my approach ;I can update it if you prefer another way.
|
Actually one nit: Should we follow the Conventional Commits guideline https://www.conventionalcommits.org/en/v1.0.0/ and update the PR title to use an action-oriented phrasing? Starting the title with a verb would help keep it consistent with the rest of the commits in the repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4587 +/- ##
==========================================
- Coverage 95.52% 95.49% -0.04%
==========================================
Files 127 127
Lines 20493 20493
Branches 1760 1759 -1
==========================================
- Hits 19576 19569 -7
- Misses 898 905 +7
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
One more thing: I just helped you update this but in the PR description, please include the word
|
Description
Related issue(s)
Fixes #4209
Testing Guide
Checklist