Closed alelom closed 1 year ago
Nice! Quick comment on the Unit Tests - think good case to be made to keep closer to code for advanced framework projects like this! The core framework code (which demands necessarily more complex functionality - and I would include in this the Adaptor and UI repos, and perhaps core functionality like versioning and serialisation etc.) should naturally have robust unit tests - and should be stored where most logical to ease development, testing and review. This is perhaps converse to our “discipline engines” which we should continue to strive to be kept as “simple as possible”, relying heavily on our data set driven unit tests and for which the compliance rules were largely originally derived I feel.
Worth opening a discussion on refining compliance for the advanced cases like this @alelom to enanle gathering thoughts and consensus ?
Have discussed with @alelom that he will put the adapter tests within the .ci
folder for this repo, as a .ci/code
solution called Verification.sln
, which contains a Verify
class (static class) in line with Versioning, Null-Handling, and Serialisation.
In addition to this, an additional project will be housed that contains @alelom console application that has allowed him to do the live-edit testing/debugging that has helped this work be developed up. This will allow us to have the best of the automated testing via the bot and during development testing 😄
Generally happy with the functionality now. From my testing on a couple of adapters it is working great.
Also, generally happy with the code, see minor comments above regarding the latest commits.
@BHoMBot check compliance
@BHoMBot check compliance
@BHoMBot check required
@BHoMBot check required
@BHoMBot check project-compliance
@BHoMBot check project-compliance
@BHoMBot check project-compliance
@BHoMBot check required
@IsakNaslundBh @alelom - Discussed with you both today but commenting for transparency and authorisation for @IsakNaslundBh that any compliance failures related SOLELY to the .ci
folder are ok to be dispensated if I'm not around to provide it 😄
@BHoMBot check copyright-compliance
@BHoMBot check compliance @BHoMBot check required
@BHoMBot check ready-to-merge
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 10483867458
Autorising the ProjectCOmpliance on the basis that only the .ci folder is affected, as agreed in comment from @FraserGreenroyd above
@BHoMBot check ready-to-merge
Depends on
https://github.com/BHoM/BHoM_Engine/pull/2974
Issues addressed by this PR
Closes #331
Test files
Use the full testing procedure for Adapters on this PR.
Additional tests are temporarily provided in the form of Unit Test within the solution. I found it extremely useful to develop this functionality by preparing tests before developing, because I could run the whole adapter workflow, debug and live-edit while debugging. I wish we could keep them where they are but, because currently this is non-compliant, they will likely be moved out in a dedicated toolkit, similarly to what we do with DiffingTests_prototypes.
Changelog
Additional comments
Because this feature is needed to fully leverage the work done by @enricoantolini in the Karamba_Toolkit, I am putting it as a high priority feature, to be merged as soon as possible. However, in depth testing is needed on this one.
Additionally, some ancillary features are needed to avoid some slowdown introduced by this PR, like https://github.com/BHoM/BHoM_Adapter/issues/329, which should be mergeable soon after this gets merged.