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
Add 1ES CI for DTFx.AS v2 #1139
Add 1ES CI for DTFx.AS v2 #1139
Changes from all commits
6807f98
7bbfb20
5341508
dd49cf6
4bc59ce
b2bd2f9
bc9a5ea
aa77bd5
56984ce
eef1d03
b45e013
5b7b1d8
aa060c2
941decc
1e1275d
15f44df
1c0c4d2
b1eec32
345fc27
e791819
01ba1b2
896fcc5
5df3df2
dd7c604
f1b0f72
de3761a
4bf3039
2e7d199
1dda1ab
be16e51
cb72151
1f56a2d
bd915ec
4d4dda0
275b3dd
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.
after upgrading DTFx.Core's
System.Diagnostics.DiagnosticSource
to 6.x, there's some ambiguous definitions here. So I needed to add this alias to disambiguateThere 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.
after upgrading DTFx.Core's
System.Diagnostics.DiagnosticSource
to 6.x, these named parameters create ambiguous method invocations here (there's multiple versions of this method with the same parameters in different orders). Using a positional argument here disambiguates the call.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.
these disabled tests have not been running for a long time, and they're failing in the main branch as well. They weren't running because they're conditionally compiled not to run in NET462, and our previous testing task in ADO was configured to run tests found in the
net462
dll.Now, the test task does discover these tests, but I've disabled them as they aren't passing. @bachuv can you please add this as a To-Fix before Dist. Tracing GA?
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.
Yes, I'll add this task to the list.
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.
newtonsoft.json is inherited by the DTFx.Core project
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.
I added this
Logging.Console
dependency because I've configured the tests to emit traces to STDOUT, which makes debugging failed ADO tests much easier. I decided to unify the dependencies to version 2.2.0 for simplicy sake and to avoid conditional compilation.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.
2.2.0 is quite old. Can we standardize to
6.0.0
as part of the v3 change?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.
I had some issues getting the code to build on 1ES with more recent versions. I'm open to upgrading, but since this is the test project, I'd like to push that to be a separate PR.
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.
we don't have a netcoreapp2.1 TFM
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.
as mentioned, I've added logging to STDOUT on our tests to facilitate debugging. The current logging library we use is pretty old, which is why we have to disable the warning here. I tried using a newer library, but then had issues with the NET462 TFM. For speed, I decided to use the old library instead, which works.
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.
FYI @cgillum, @jviau
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.
Do you mean the
.net462
flavor of the tests are not discoverable? The .NET Core flavor is working?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.
Can we add comments here with reminders to update these dependencies?
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.
Yes, just did here -> bc9a5ea