Skip to content
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

[API Implementation]: Add Microseconds and Nanoseconds to TimeStamp, DateTime, DateTimeOffset, and TimeOnly #67666

Merged
merged 15 commits into from
Apr 14, 2022

Conversation

deeprobin
Copy link
Contributor

Proposal implementation of #23799 (closes #23799)

Proposal

namespace System {
    public struct DateTime {
        public DateTime(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond);
        public DateTime(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, DateTimeKind kind);
        public DateTime(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, Calendar calendar);
        public int Microsecond { get; }
        public int Nanosecond { get; }
        public DateTime AddMicroseconds(double value);
    }
    public struct DateTimeOffset {
        public DateTimeOffset(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, TimeSpan offset);
        public DateTimeOffset(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, TimeSpan offset, Calendar calendar);
        public int Microsecond { get; }
        public int Nanosecond { get; }
        public DateTimeOffset AddMicroseconds(double value);
    }
    public struct TimeSpan {
        public const long TicksPerMicrosecond;
        public const long NanosecondsPerTick;
        public TimeSpan(int days, int hours, int minutes, int seconds, int milliseconds, int microseconds);
        public int Microseconds { get; }
        public int Nanoseconds { get; }
        public double TotalMicroseconds { get; }
        public double TotalNanoseconds { get; }
        public static TimeSpan FromMicroseconds(double value);
    }
    public struct TimeOnly {
        public TimeOnly(int day, int hour, int minute, int second, int millisecond, int microsecond);
        public int Microsecond { get; }
        public int Nanosecond { get; }
    }
}

Current state of implementation

  • Proposal logic / implementation
  • Ref Assembly
  • Tests

/cc @tarekgh
/cc @ChristopherHaws

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 6, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@deeprobin
Copy link
Contributor Author

It should be mentioned that I have taken the documentation comments from the current documentation and adapted them accordingly.

I have extended the current test cases accordingly. I assume that these are not flaky.

@ghost
Copy link

ghost commented Apr 6, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Proposal implementation of #23799 (closes #23799)

Proposal

namespace System {
    public struct DateTime {
        public DateTime(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond);
        public DateTime(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, DateTimeKind kind);
        public DateTime(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, Calendar calendar);
        public int Microsecond { get; }
        public int Nanosecond { get; }
        public DateTime AddMicroseconds(double value);
    }
    public struct DateTimeOffset {
        public DateTimeOffset(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, TimeSpan offset);
        public DateTimeOffset(int year, int month, int day, int hour, int minute, int second, int millisecond, int microsecond, TimeSpan offset, Calendar calendar);
        public int Microsecond { get; }
        public int Nanosecond { get; }
        public DateTimeOffset AddMicroseconds(double value);
    }
    public struct TimeSpan {
        public const long TicksPerMicrosecond;
        public const long NanosecondsPerTick;
        public TimeSpan(int days, int hours, int minutes, int seconds, int milliseconds, int microseconds);
        public int Microseconds { get; }
        public int Nanoseconds { get; }
        public double TotalMicroseconds { get; }
        public double TotalNanoseconds { get; }
        public static TimeSpan FromMicroseconds(double value);
    }
    public struct TimeOnly {
        public TimeOnly(int day, int hour, int minute, int second, int millisecond, int microsecond);
        public int Microsecond { get; }
        public int Nanosecond { get; }
    }
}

Current state of implementation

  • Proposal logic / implementation
  • Ref Assembly
  • Tests

/cc @tarekgh
/cc @ChristopherHaws

Author: deeprobin
Assignees: -
Labels:

area-System.Runtime, new-api-needs-documentation, community-contribution

Milestone: -

@tarekgh tarekgh self-assigned this Apr 6, 2022
@tarekgh
Copy link
Member

tarekgh commented Apr 6, 2022

@deeprobin I'll try to take a look at this PR later today and will get back to you.

@danmoseley
Copy link
Member

@deeprobin you might be interested to check that your tests are giving full code coverage of your code:

## Code coverage with System.Private.CoreLib code

@deeprobin
Copy link
Contributor Author

@danmoseley
Thanks. I can take a look at that after the other open questions have been clarified here :).

@tarekgh
Copy link
Member

tarekgh commented Apr 12, 2022

Could you please add tests for TimeSpan new APIs? Also, when writing these tests please ensure using negative values in addition to positive values.

@tarekgh
Copy link
Member

tarekgh commented Apr 12, 2022

@deeprobin I left some comments I hope you can address them soon. After that we should be good to go. great work!

@joperezr
Copy link
Member

joperezr commented Apr 12, 2022

/azp run runtime (Rerunning CI in order to fetch changes merged in main which have fixed the Regex test)

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Apr 12, 2022
@tarekgh
Copy link
Member

tarekgh commented Apr 13, 2022

@deeprobin will be able to address the remaining feedback soon?

@deeprobin deeprobin requested a review from tarekgh April 13, 2022 18:11
@tarekgh
Copy link
Member

tarekgh commented Apr 13, 2022

The failing test is unrelated and is tracked by the issue #67878

@tarekgh tarekgh merged commit b2ed250 into dotnet:main Apr 14, 2022
@ChristopherHaws
Copy link

🎉 Thanks for implementing this @deeprobin!

radical added a commit to radical/runtime that referenced this pull request Apr 14, 2022
This test broke because it was checking for the number of members on
`System.TimeSpan`, and that changed with
dotnet#67666 , which added new members
like `TotalNanoseconds`.

The test shouldn't depend on this number anyway, so remove that.

```
  Failed DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(line: 137, col: 12, method_name: "MethodWithLocalsForToStringTest", call_other: False, invoke_async: False) [758 ms]
  Error Message:
   [ts_props] Number of fields don't match, Expected: 12, Actual: 16
Expected: True
Actual:   False
  Stack Trace:
     at DebuggerTests.DebuggerTestBase.CheckProps(JToken actual, Object exp_o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 800
   at DebuggerTests.DebuggerTestBase.CompareObjectPropertiesFor(JToken locals, String name, Object o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 908
   at DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(Int32 line, Int32 col, String method_name, Boolean call_other, Boolean invoke_async) in /_/src/mono/wasm/debugger/DebuggerTestSuite/MiscTests.cs:line 559
```
radical added a commit to thaystg/runtime that referenced this pull request Apr 15, 2022
This test broke because it was checking for the number of members on
`System.TimeSpan`, and that changed with
dotnet#67666 , which added new members
like `TotalNanoseconds`.

The test shouldn't depend on this number anyway, so remove that.

```
  Failed DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(line: 137, col: 12, method_name: "MethodWithLocalsForToStringTest", call_other: False, invoke_async: False) [758 ms]
  Error Message:
   [ts_props] Number of fields don't match, Expected: 12, Actual: 16
Expected: True
Actual:   False
  Stack Trace:
     at DebuggerTests.DebuggerTestBase.CheckProps(JToken actual, Object exp_o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 800
   at DebuggerTests.DebuggerTestBase.CompareObjectPropertiesFor(JToken locals, String name, Object o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 908
   at DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(Int32 line, Int32 col, String method_name, Boolean call_other, Boolean invoke_async) in /_/src/mono/wasm/debugger/DebuggerTestSuite/MiscTests.cs:line 559
```
radical added a commit that referenced this pull request Apr 15, 2022
This test broke because it was checking for the number of members on
`System.TimeSpan`, and that changed with
#67666 , which added new members
like `TotalNanoseconds`.

The test shouldn't depend on this number anyway, so remove that.

```
  Failed DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(line: 137, col: 12, method_name: "MethodWithLocalsForToStringTest", call_other: False, invoke_async: False) [758 ms]
  Error Message:
   [ts_props] Number of fields don't match, Expected: 12, Actual: 16
Expected: True
Actual:   False
  Stack Trace:
     at DebuggerTests.DebuggerTestBase.CheckProps(JToken actual, Object exp_o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 800
   at DebuggerTests.DebuggerTestBase.CompareObjectPropertiesFor(JToken locals, String name, Object o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 908
   at DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(Int32 line, Int32 col, String method_name, Boolean call_other, Boolean invoke_async) in /_/src/mono/wasm/debugger/DebuggerTestSuite/MiscTests.cs:line 559
```
thaystg added a commit that referenced this pull request Apr 26, 2022
* First compiling and working version just proxying messages.

* almost working, already showing the cs files

* Working on firefox.

* Use internal e not public.

* Debugging on firefox working.

* Working after the merge

* Keep the TcpListener open and use a random port.

* Show null value.

* - Show JS Callstack
- Skip properties
- Get array value from evaluateAsyncJS and not use the preview value.

* Fix compilation

* Infrastructure to run debugger tests.

* fix merge

* run test.

* Skipping tests that are not passing on Firefox.

* Skipping tests that are not passing on Firefox.

* Passing 13 steppingtests.

* Passing 13 steppingtests.

* Passing 13 steppingtests.

* Failed:     0, Passed:    39, Skipped:   203, Total:   242, Duration: 5 m 6 s - DebuggerTestSuite.dll (net6.0)

* Failed:     0, Passed:    66, Skipped:   195, Total:   261, Duration: 9 m 29 s - DebuggerTestSuite.dll (net6.0)

* Using ConditionalTheory and ConditionalFact implemented by @radical.

* Fixing side effect.

* Implemented conditional breakpoints.
Failed:     0, Passed:    74, Skipped:   189, Total:   263, Duration: 8 m 41 s - DebuggerTestSuite.dll (net6.0)

* Fix special characters and pointers.

Failed:     0, Passed:   116, Skipped:   177, Total:   293

* Fix merge

* Run debugger-tests on firefox using codespace

* Starting firefox correctly not stopping in the breakpoint yet.

* Remove unnecessary change

* Fix pause behavior (now showing correctly, pause on breakpoint, pause while stepping)
Start implementing evaluate expressions, working correctly on VSCode.

* Fix local tests.

* Fix missing )

* Passing 190 tests, evaluate expressions working.

* Remove Task.Delays.
Move some attributes from FirefoxMonoProxy to FirefoxExecutionContext.

* Fix container creation

* Trying to run firefox tests on CI.

* Moving file to the right place.

* Trying to run debugger-tests using firefox on CI.

* fixing path

* Missing url to download firefox on helix.

* On run the tests only on linux.

* Trying to download firefox on helix.

* fix error on helix-wasm.targets.

* trying to fix ci

* trying to install firefox on helix.

* Fixing firefox path

* Fix debugger tests on firefox

* fixing profile path

* Install libdbus-glib-1-2 on docker and on codespace

* Trying to run using firefox on CI

* update docker image

* Adding more messages to see errors on CI

* Trying to make it work on CI

* Real test on CI

* Trying to use the firefox machine only to run firefox tests
Retrying connection to Proxy
Remove extra messages added to help to fix CI

* Fix CI

* Fix CI

* Fix CI.

* Remove unnecessary changes.

* Using machine with sudo installed

* Addressing @lewing comments

* Fix run tests on codespace
Using image with python3

* Use default image to build and new image only to run firefox tests

* Fix unrelated change

* Fix ci

* check python version

* Print python versions

* Using image with PIP installed

* Using image with pip updated

* Remove unrelated changes
Increase time to wait for firefox to be ready

* Trying to fix evaluate tests.

* Fix evaluateoncallframe tests

* Trying to fix evaluation tests.

* trying to fix evaluateoncallframetests

* fiz evaluateoncallframetests

* Trying to kill firefox to avoid errors.

* Trying to fix EvaluateOnCallFrameTests

* Fix CI

* Remove failing test

* Fix misctests

* Fix other build errors.

* Trying to fix CI.

* Fix CI

* Remove unecessary message.

* Update src/tests/BuildWasmApps/Wasm.Debugger.Tests/Wasm.Debugger.Tests.csproj

Co-authored-by: Ankit Jain <radical@gmail.com>

* Addressing @radical comments

* Merge error while accept @radical suggestion

* Merge error while accept @radical suggestion

* Update src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs

Co-authored-by: Ankit Jain <radical@gmail.com>

* Apply suggestions from code review

Co-authored-by: Ankit Jain <radical@gmail.com>

* Addressing @radical comments

* Abort the tcp connection if the proxy throws an exception

* Refactor a bit

* Use more compile time checks for chrome vs firefox

* fix pipeline

* Make debugger job names unique by including the browser

* fix runtime-wasm pipeline

* fix firefox ci job

* split into more files

* cleanup

* Add support for running chrome, and firefox tests in the same job

* fix yml

* fix build

* fix build

* fix windows build

* Don't delete profile folder nor pkill firefox

* Delete and create a new profile folder for each execution

* fix helix command line

* [wasm][debugger] Fix tests broken on 'main'

This test broke because it was checking for the number of members on
`System.TimeSpan`, and that changed with
#67666 , which added new members
like `TotalNanoseconds`.

The test shouldn't depend on this number anyway, so remove that.

```
  Failed DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(line: 137, col: 12, method_name: "MethodWithLocalsForToStringTest", call_other: False, invoke_async: False) [758 ms]
  Error Message:
   [ts_props] Number of fields don't match, Expected: 12, Actual: 16
Expected: True
Actual:   False
  Stack Trace:
     at DebuggerTests.DebuggerTestBase.CheckProps(JToken actual, Object exp_o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 800
   at DebuggerTests.DebuggerTestBase.CompareObjectPropertiesFor(JToken locals, String name, Object o, String label, Int32 num_fields) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 908
   at DebuggerTests.MiscTests.InspectLocalsForToStringDescriptions(Int32 line, Int32 col, String method_name, Boolean call_other, Boolean invoke_async) in /_/src/mono/wasm/debugger/DebuggerTestSuite/MiscTests.cs:line 559
```

* wip

* big refactor

* chrome runs

* ff runs

* ff runs

* cleanup

* cleanup

* cleanup

* change console verbosity to info, for proxy

* More refactoring

* More refactoring, and fix some issues with connections, and other
cleanup

* some cleanup

* fix file name

* Improve cleanup after tests

* some refactoring, fixing some hangs, faster failures etc

* Fix BrowserCrash test for chrome

* fix up logging

* Improve error handling for the proxy running independently

* fix debugging from vscode

* proxy host: add --log-path for logs

* support canceling for the proxy host too, and distinguish different instances of the proxy

* Fix debugger after refreshing the debugged page.

* Fixing chrome debugging.

* Fix startup to work on chrome and also on firefox.

Co-authored-by: Ankit Jain <radical@gmail.com>
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DateTime community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Microseconds and Nanoseconds to TimeStamp, DateTime, DateTimeOffset, and TimeOnly
6 participants