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

Add initial unit tests #71

Closed
wants to merge 51 commits into from
Closed

Add initial unit tests #71

wants to merge 51 commits into from

Conversation

networkfusion
Copy link
Member

@networkfusion networkfusion commented Jul 28, 2022

Description

This library did not contain unit tests.
Adds Initial tests using big brother implementation from: https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.FileSystem.Primitives/tests

Further tests "could" be obtained from https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.FileSystem/tests

Motivation and Context

  • Project required some unit tests.
  • Library should start to align with .Net as far as possible.

How Has This Been Tested?

All changes should be non intrusive.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

Add fixes to enums.
@networkfusion
Copy link
Member Author

networkfusion commented Jul 28, 2022

Note that FileAttributes are (actually (or can be up to)) 3 bytes in length, so probably should be handling Int32 (fixed original enum from unit test) and may possibily require a change in the native layer (currently returns a byte).

Reasoning is that we should support it (even if FatFS does not) for future support with FileX etc.

Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, few comments

</RunConfiguration>
<nanoFrameworkAdapter>
<Logging>None</Logging>
<IsRealHardware>False</IsRealHardware>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the tests running in the virtual device? Is the System.IO.File having a native implementation supported there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the tests are just checking that the enumerations are correct (in theory) aligned to the full fat .NET implementation. We need to make sure these can align with the native etc..

@josesimoes
Copy link
Member

OK and kudos for adding unit tests! Always welcome.

OK to for making the enum type match the full .NET ones.
Now, I don't see a point on adding all these attributes that we can't support...
If this is for code sharing use cases (with code files shared in full .NET projects) adding them will give the wrong impression that code is fine.
On the contrary if we don't have them, build will break and the developer will have to handle those situations.

@networkfusion
Copy link
Member Author

OK and kudos for adding unit tests! Always welcome.
💯

If this is for code sharing use cases (with code files shared in full .NET projects) adding them will give the wrong impression that code is fine. On the contrary if we don't have them, build will break and the developer will have to handle those situations.

I see your point, but would rather see a not supported exception for them.
For the sake of this PR, I think a comprimise is to comment them out and add a note (which I have now done).

We can always remove them on the next pass as more unit tests are added.

@josesimoes
Copy link
Member

I see your point, but would rather see a not supported exception for them.

Can we throw an exception for a non existing enum item? 😯 I don't think I've seen that... 🤔
The only usable way I see is just not having them there.

For the sake of this PR, I think a comprimise is to comment them out and add a note (which I have now done).
We can always remove them on the next pass as more unit tests are added.
On a quick look: we won't ever have support for those enums. I guess that was the reason for not having them there in the 1st place.

@networkfusion
Copy link
Member Author

networkfusion commented Apr 26, 2023

I see your point, but would rather see a not supported exception for them.

Can we throw an exception for a non existing enum item? 😯 I don't think I've seen that... 🤔 The only usable way I see is just not having them there.

For the sake of this PR, I think a comprimise is to comment them out and add a note (which I have now done).
We can always remove them on the next pass as more unit tests are added.
On a quick look: we won't ever have support for those enums. I guess that was the reason for not having them there in the 1st place.

Pesudo code:

        /// <summary>
        /// Sets the specified FileAttributes of the file on the specified path.
        /// </summary>
        /// <param name="path">The path to the file.</param>
        /// <param name="fileAttributes">A bitwise combination of the enumeration values.</param>
        public static void SetAttributes(string path, FileAttributes fileAttributes)
        {
            if ((byte)fileAttributes > (byte)FileAttributes.Archive)
            {
              throw new NotSupportedException();
            }
            else
            {
                SetAttributesNative(path, (byte)fileAttributes);
            }
        }

And I still think that the get should be compatible (even if it just ignores) with all of them...

And, there is nothing stopping someone just specifiying a byte instead of the enum anyway, so should possibily be handled in native.

Also noting that we have never supported file share attributes, yet still include them?!

But this is why we need to have some unit tests!

@josesimoes
Copy link
Member

@networkfusion I get that you can add those checks on the library code. My concern is with the code calling our library. We can't enforce those to be checked.
Simplest fix for this and not having to worry about it: don't have them in the 1st place.

Any user code using those enums that have no implementation it will build succesfully but those can not be honoured (whathever they are doing).

@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@nfbot nfbot added Type: enhancement New feature or request Type: Documentation Improvements or additions to documentation labels May 8, 2023
@networkfusion
Copy link
Member Author

Closing this as #98 is adding them.

@nfbot nfbot added invalid and removed Type: Documentation Improvements or additions to documentation Type: enhancement New feature or request Type: Unit Tests labels Apr 13, 2024
@networkfusion networkfusion deleted the add-unit-tests branch April 13, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants