Skip to content
This repository was archived by the owner on Nov 20, 2023. It is now read-only.

Fixed issue #620#621

Merged
JunTaoLuo merged 4 commits into
dotnet:masterfrom
ckadluba:issue-620
Aug 19, 2020
Merged

Fixed issue #620#621
JunTaoLuo merged 4 commits into
dotnet:masterfrom
ckadluba:issue-620

Conversation

@ckadluba

@ckadluba ckadluba commented Aug 7, 2020

Copy link
Copy Markdown
Contributor
  • Produce a useful error message if the path of a project file is incorrect in tye.yaml.
  • Added a test.

@ckadluba
Fixed issue dotnet#620
8fba0f7 
* Produce a useful error message if the path of a project file is incorrect in tye.yaml.
* Added a test.
@ckadluba

ckadluba commented Aug 7, 2020

Copy link
Copy Markdown
Contributor Author

Here is a small fix for issue #620 including a test. I humbly hope you find it useful.

@ckadluba

ckadluba commented Aug 7, 2020

Copy link
Copy Markdown
Contributor Author

Hmm .. I'm not sure why the Ubuntu build fails now. In the build logs there is this message about an ArgumentOutOfRangeException.

Unhandled exception. System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'chunkLength')
   at System.Text.StringBuilder.ToString()
   at Microsoft.Tye.ProcessUtil.<>c__DisplayClass1_0.<RunAsync>b__2(Object _, EventArgs e) in /_/src/Microsoft.Tye.Core/ProcessUtil.cs:line 103
   at System.Diagnostics.Process.RaiseOnExited()
   at System.Diagnostics.Process.CompletionCallback(Object waitHandleContext, Boolean wasSignaled)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
Aborted (core dumped)

I'll investigate this later.

ckadluba added 2 commits August 7, 2020 20:17
@ckadluba
Check full exception message in test and fixed test name.
e167357
@ckadluba
More deterministic behavior considering directory check and test exec…
96e8c6b 
…ution.

Moved directory check one level higher from EnsureMSBuildRegistered() to the caller ReadProjectDetailsAsync(). This means that this code is always executed. In EnsureMSBuildRegistered() it was only executed once per proces because of the static field "registered". The placement of the check after evaluation of the the field "registered" was the reason why the test WrongProjectPathProducesCorrectErrorMessage worked when executed alone but not when any other test that called EnsureMSBuildRegistered() was executed before.

With this change the directory check is always executed now, even if the EnsureMSBuildRegistered() was already executed successfully. But the performance impact should not be significant while the type of generated error message is more deterministic and not dependent on execution order.
@ckadluba

ckadluba commented Aug 8, 2020

Copy link
Copy Markdown
Contributor Author

It looks like my code change and test seem to work stable now but in the Ubuntu CI build there was an error in an unrelated test named MultiRepo_Works showing a network connection problem.

System.Net.Http.HttpRequestException : Connection refused\n---- System.Net.Sockets.SocketException : Connection refused

Could this be a transient infrastructure problem?

@glennc

glennc commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

Re-running the tests had them all pass. @jkotalik have you noticed these being flaky in the past?

@ckadluba

ckadluba commented Aug 17, 2020

Copy link
Copy Markdown
Contributor Author

Hi guys! Any news on this?

@JunTaoLuo JunTaoLuo merged commit a4cf135 into dotnet:master Aug 19, 2020
@JunTaoLuo

JunTaoLuo commented Aug 19, 2020

Copy link
Copy Markdown

Thanks for the fix!

@ckadluba ckadluba deleted the issue-620 branch August 21, 2020 19:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidfowl davidfowl Awaiting requested review from davidfowl

@jkotalik jkotalik Awaiting requested review from jkotalik

@rynowak rynowak Awaiting requested review from rynowak

1 more reviewer

@JunTaoLuo JunTaoLuo JunTaoLuo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ckadluba @glennc @JunTaoLuo