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.

Sign up for GitHub

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

Jump to bottom

Add Xdt module to FAKE 5 #2218

Merged
merged 5 commits into from
Jan 12, 2019
Merged

Add Xdt module to FAKE 5 #2218

merged 5 commits into from
Jan 12, 2019

Conversation

panesofglass
Copy link
Contributor

@panesofglass panesofglass commented Nov 30, 2018 •
edited
Loading

Description

Migrate Fake.XDTHelper to FAKE 5 module Fake.DotNet.Xdt.

TODO

Feel free to open the PR and ask for help

  • New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)

  • unit or integration test exists (or short reasoning why it doesn't make sense)

    Note: Consider using the CreateProcess API which can be tested more easily, see https:/fsharp/FAKE/pull/2131/files#diff-4fb4a77e110fbbe8210205dfe022389b for an example (the changes in the DotNet.Testing.NUnit module)

  • boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)

  • (if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.

  • (if new module) the module is in the correct namespace

  • (if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)

  • Fake 5 API guideline is honored

@panesofglass panesofglass force-pushed the feature/xdt branch 2 times, most recently from fed0424 to ec7d60a Compare November 30, 2018 19:22
@panesofglass
Copy link
Contributor Author

@matthid any idea why tests are failing? Paket updated a bunch of dependencies, but I wasn't sure how to not touch them without editing the paket.lock by hand.

@matthid
Copy link
Member

matthid commented Dec 2, 2018

@panesofglass yes this error happens when the runner has a different version of paket.core than the rest of the packages. Make sure all paket.core versions match up in the lockfile.

@panesofglass
Copy link
Contributor Author

Thanks, @MattiD.

@panesofglass
Copy link
Contributor Author

@matthid looks like only CircleCI failed this time.

@matthid
Copy link
Member

matthid commented Dec 3, 2018 

[17:19:17 ERR] Fake.DotNet.Xdt.Tests/when transforming files with config name errored in 00:00:00.2530000 <Expecto>
System.IO.IOException: The process cannot access the file '/build/src/test/Fake.Core.UnitTests/TestFiles/Fake.DotNet.Xdt.Files/web.config' because it is being used by another process.
   at System.IO.FileStream.Init(FileMode mode, FileShare share)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
   at System.IO.FileSystem.CopyFile(String sourceFullPath, String destFullPath, Boolean overwrite)
   at System.IO.File.Copy(String sourceFileName, String destFileName, Boolean overwrite)
   at [email protected](Unit unitVar)
   at [email protected](Unit unitVar) in /Users/h/dev/logibit/expecto/Expecto/Expecto.fs:line 928
   at [email protected](AsyncParams`1 args)

looks related to this PR.

@panesofglass
Copy link
Contributor Author

@matthid parallel tests, perhaps?

@matthid
Copy link
Member

matthid commented Dec 3, 2018

Yes by default stuff runs parallel and I feel like unit tests should work when run in parallel ;). If for some strong reason they need to run sequential there is a sequentialTests function in expecto.

@panesofglass
Copy link
Contributor Author

Thanks, @matthid. Looks like that fixed it.

matthid
matthid reviewed Dec 3, 2018
View reviewed changes
src/test/Fake.Core.UnitTests/Fake.DotNet.Xdt.fs Outdated

[<Tests>]
let tests =
testSequenced <| testList "Fake.DotNet.Xdt.Tests" [
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to just copy the test-files or create unique names to remove the need to have them running sequentially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what you mean. Can you give an example? I copied the tests that were provided for the old XDTHelper and converted them to use Expecto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One challenge for two of the functions is that they modify a file in-place, so I'm not sure how to resolve that without adding another dependency like a virtual file system.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just copy the file in the test? Afaik read access can be shared, just writing is problematic...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to make the change, but I'm not sure what exactly to do here. Do you have a specific suggestion?

@panesofglass
Copy link
Contributor Author

@matthid, does my last commit address your concerns?

@panesofglass panesofglass force-pushed the feature/xdt branch 2 times, most recently from 8984924 to cbcb030 Compare December 10, 2018 19:50
@panesofglass
Copy link
Contributor Author

panesofglass commented Dec 10, 2018 •
edited
Loading

@matthid, would you mind re-running the AppVeyor build? The failure looks like a build server error.

matthid
matthid previously approved these changes Dec 18, 2018
View reviewed changes
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

@matthid, does my last commit address your concerns?

Almost, I'd try to change it to use temporary file-names OR make each test use it's own directory. I can change that myself but it will take a couple of days.

Anyway, thanks a lot for taking care of this module.

src/app/Fake.DotNet.Xdt/Xdt.fs Outdated
let private xdtTag = KnownTags.Task "XDT"

/// Integrates XDT logging into FAKE logging.
type FakeXmlTransformationLogger() =
Copy link
Member

Choose a reason for hiding this comment

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

Can/should we make that internal?

@panesofglass
Copy link
Contributor Author

@matthid any chance you can merge this? It's blocking us from using FAKE 5 on our web projects.

@matthid
Copy link
Member

matthid commented Jan 10, 2019

Yes sorry, I just wanted to make FakeXmlTransformationLogger internal and take a look at the tests but I didn't have the opportunity yet.

@panesofglass
Copy link
Contributor Author

@matthid, so long as change to making FakeXmlTransformationLogger internal doesn't break the build, I think this should be good. Thanks for your feedback!

@matthid matthid merged commit cd399c7 into fsprojects:release/next Jan 12, 2019
@matthid
Copy link
Member

matthid commented Jan 12, 2019

@panesofglass please see/review my changes in 570e10b
Will release soonish.

@matthid
Copy link
Member

matthid commented Jan 12, 2019

Again thanks! Sorry for the delay.

@matthid matthid mentioned this pull request Jan 12, 2019
Merged
@panesofglass panesofglass deleted the feature/xdt branch January 12, 2019 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid left review comments

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
@panesofglass @matthid @tachyus-ryan