Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
System tests should cleanup after themselves #2216
System tests should cleanup after themselves #2216
Changes from 50 commits
d993866
3b01d28
fb156de
960777b
68a7fdd
4f059e6
81048a7
a505c0d
56f0e9a
c458433
e356ac8
123c7f9
d7b2840
e1f2ca2
4ccaad5
c5995dd
a0fa936
f746a0b
885a28a
f334c76
e11726f
70cb522
d87f789
0b475ac
3853c8a
0159d2c
c7de80c
54baa2d
ebe3570
663340f
9d59e1a
fe2c72b
46677f7
21bf9aa
a9d2865
52361c2
309be62
585bb75
a83444a
bc1ffa2
8ccb8f4
842ee6b
9e8f22e
1c7a5b2
83faac9
6c7064b
deb5cfa
cefbff1
aaa43b8
201ecc1
4a37668
36a19ca
5e40b67
2d1c9be
d89d3b1
ae20141
54e8d99
9de05a2
f9f3b16
3b008cb
4a96709
04b92c8
5f1d974
6a1dc9f
a628144
7f50352
66f5e11
73e9201
0a32123
e3805fd
b2a3bac
336d3e4
7068b6c
a2cb59e
f7f6696
8229c76
cacdd3a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using generics, it's advisable to keep the name short, and ideally something different to an existing interface.
you could get the same benefits by doing
<T extends ITestPropertiesSchema>
and use the value ofT
as the desired type for this properti (e.g.systemTestProperties: T
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awharn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the value before was TestPropertiesSchema, do we need to make that change? I am also a bit confused with the difference between generics and interfaces in this particular context.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, these are Typescript generics which do not have any impact on users.
Having the
ITestPropertiesSchema
will cause confusion as we might expect the interface to be taking effect when it won't.The reason why
TestPropertiesSchema
never caused problems was because it was just the name that we gave it (which is equivalent to using a single letter, likeT
)Hence the suggestion of
T extends ITestPropertiesSchema
to get the benefits of the interface 😋Feel free to ignore this comment if you feel I'm completely off base here (because, honestly, I might be) 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i just reverted changes to the file... is that alright? commit: 2d1c9be
@zFernand0 @t1m0thyj