Update build instructions

Description
Update build instructions for windows examples and fix minor errors in instructions

Checklist to comply with before moving PR from draft:
PR completeness and readability

[ ] I have reviewed my changes thoroughly before submitting this pull request.
[ ] I have commented my code, particularly in hard-to-understand areas.
[ ] I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
[ ] Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
[ ] I have added a respective label(s) to PR if I have a permission for that.
[ ] I have resolved any merge conflicts that might occur with the base branch.

Testing

[ ] I have run it locally and tested the changes extensively.
[ ] All CI jobs are green or I have provided justification why they aren't.
[ ] I have extended testing suite if new functionality was introduced in this PR.

@Alexandr-Solovev @Vika-F could you review these changes, please


issue with  AttributeError: 'float' object has no attribute 'item'

Hi,
I encounter this problem when running main.py:
File "C:\Users\Documents\Python Scripts\Sentiment-Analysis-in-Event-Driven-Stock-Price-Movement-Prediction-master\util.py", line 31, in
set_scale = [parameter.data.std().item() for parameter in model.parameters()]
AttributeError: 'float' object has no attribute 'item'
error seems to came from util.py
Any idea on how to solve it?
Cheers

You are probably using older version of Pytorch

Thanks.


Sliver*List (Extend, Prototype and others) with different main axis.

Use case
I'm trying to have nested list with different scrollDirection inside customScrollView or NestedScrollView.
I known that we can use SliverToBoxAdapter and then wrap the listview as the child but it's not really convenient and requires a lot of boilerplates and also if you don't known the height of your item, it's even more complicated.
Proposal
Provide a scrollDirection parameter to the silverlist widgets or a widget that allow to change the main axis of a child of type sliverlist widget.

Provide a scrollDirection parameter to the silverlist widgets

Hi @bitsydarel, the scroll direction is provided to the SliverList in the SliverConstraints. Can you share more about your use case and what is not working? Some sample code would be very helpful in identifying what we can do here to help. Thanks!


[B17] [possibly intentional] Primal-Vis Crystal Clusters can't turn into Vitium Crystal Clusters anymore

As title. I realize that this might well be by choice and I don't have a preferable version of how this should work, but I thought I'd bring it up just in case this got dropped by mistake.

This seems intentional - the Thaumonomicon used to have a blurb about how Clusters could become tainted by Flux, but that paragraph has been removed. Apparently, that Flux-removal mechanic has been almost entirely replaced by Flux Condensers. (Vitium Crystals can now (technically) absorb Flux, but the necessary conditions make it a little impractical in most cases.)
Personally, I think the tainting-crystals mechanic should have been kept - as a flux-management method, it had completely different costs, process, and degree of automatability than Flux Condensers. IMO, the two methods could have coexisted, being of differing usefulness in different situations.

good point, thanks. leaving this open to prevent dupes, close at will.

I had to think a bit to try and remember why I changed it, then I recalled the issue I had in one test world. I was messing around with flux and this basically caused all worldspawn crystals within several chunks to switch to flux crystals which in turn caused the flux levels to drop very quickly as they spread.
I like the idea of crystals being corrupted by flux as well, but it was making flux a bit too easy to deal with and possibly screwing over people by removing their access to normal crystals.
I will give it some more thought though. Maybe there is a middle ground somewhere.

This issue was moved to Azanor/thaumcraft-suggestions#336


ERROR: "check-db" exited with 1.

The latest umami update with the postgres latest database is not working same with mysql as database in some server not all.
yarn run v1.22.19
$ npm-run-all check-db update-tracker start-server
$ node scripts/check-db.js
✓ DATABASE_URL is defined.
✗ Unable to connect to the database.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "check-db" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v1.22.19

That step simply tries to connect to the database. Did you change your DATABASE_URL at all?

That step simply tries to connect to the database. Did you change your DATABASE_URL at all?

Everything works perfectly. We only have a problem on some Synology Servers from the 20+ series
Reference: https://mariushosting.com/how-to-install-umami-on-your-synology-nas/
The installation is smooth on all NAS devices, with the exception of 20+series on which it's still not working and can't connect to the db. I have received 50 reports in the last month from different users with 20+ series NAS. I don't know what is causing the issue, the only thing they have in common is the Intel Celeron J4125 CPU.

@mariushosting i'm not familiar with your environment but i had the same issue today. For some reason my environment changes the DATABASE_URL from mysql://xxx to mysql2://xxx.
Are you able to run commands on your system? Try printenv and verify the correct format of the DATABASE_URL env variable.
@mikecao is there an alternative solution to configure the connection? Like a way to not use a DATABASE_URL but configure the values seperately like DATABASE_HOST,DATABASE_USER etc?

@tschannik Even if we split up the connection details, you'd still have to define environment variables. You'll have to figure out why your environment variables are not showing up.
@mariushosting Is it possible the 20+ has a firewall preventing external connections?

@mikecao Well the problem is that in my environment i have no control over the DATABASE_URL env variable. It is automatically set by the system (even with the correct values) but it starts with mysql2:// instead of mysql://. So the script check-db.js verifies the existence of DATABASE_URL with is successful but the next step prisma.$connect() fails because of an unsupported url.
Unfortunately i'm not able to rewrite the DATABASE_URL to sth else. To make it work i hat to create my own dockerfile with a customized entrypoint so i could "rename" the variable.
My case is probably very specific. So i would agree that reading the values from different variables is not a "important" feature 👍

The firewall settings is identical to other NAS. Only the 20+ models are getting trouble.

@tschannik Can you try setting DATABASE_TYPE? That is another variable that we check.

@mariushosting I only have a synology 19+ so I can't troubleshoot it

hey @mariushosting, i run umami without issue on a 1517+ using the postgres db. maybe try that?
here's my docker-compose:
--- version: '3' services:   umami:     container_name: umami     image: ghcr.io/umami-software/umami:postgresql-latest     ports:       - "3000:3000"     environment:       DATABASE_URL: postgresql://umami:umami@db:5432/umami       DATABASE_TYPE: postgresql       HASH_SALT: hash-key-here       PUID: puid-here       PGID: admin-pgid-here       TRACKER_SCRIPT_NAME: tracker-name-here     depends_on:       - db     restart: always   db:     container_name: umami-db     image: postgres:12-alpine     environment:       POSTGRES_DB: umami       POSTGRES_USER: umami       POSTGRES_PASSWORD: umami     volumes:       - /volume1/docker/umami/sql/schema.postgresql.sql:/docker-entrypoint-initdb.d/schema.postgresql.sql:ro       - /volume1/docker/umami/umami-db-data:/var/lib/postgresql/data     restart: always volumes:   umami-db-data:
big fan of your work!

hey @mariushosting, i run umami without issue on a 1517+ using the postgres db. maybe try that?
here's my docker-compose: --- version: '3' services: umami: container_name: umami image: ghcr.io/umami-software/umami:postgresql-latest ports: - "3000:3000" environment: DATABASE_URL: postgresql://umami:umami@db:5432/umami DATABASE_TYPE: postgresql HASH_SALT: hash-key-here PUID: puid-here PGID: admin-pgid-here TRACKER_SCRIPT_NAME: tracker-name-here depends_on: - db restart: always db: container_name: umami-db image: postgres:12-alpine environment: POSTGRES_DB: umami POSTGRES_USER: umami POSTGRES_PASSWORD: umami volumes: - /volume1/docker/umami/sql/schema.postgresql.sql:/docker-entrypoint-initdb.d/schema.postgresql.sql:ro - /volume1/docker/umami/umami-db-data:/var/lib/postgresql/data restart: always volumes: umami-db-data:
big fan of your work!

Hello wheeleran, as already write at the beginning of this post the problem is only with NAS series model x20 not x17.

In case this is the issue for anyone else reading this thread: I discovered that if my DATABASE_URL has specific characters, such as the # character in a password, then yarn check-db fails:
yarn run v1.22.19
$ node scripts/check-db.js
✓ DATABASE_URL is defined.
✗ Unable to connect to the database.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.


German sentences that should be supported

If you don't feel comfortable contributing to the YAML in this repository, you can post your German sentences that match our supported intents below.

For covers, I usually say:

Öffne/schließe den/die Rolladen in Pauls Zimmer
Öffne/schließe den/die Rolladen im Wohnzimmer
Öffne/schließe alle Rolladen
Rollladen auf x Prozent
Rollladen in Pauls Zimmer auf x Prozent
Alle Rollladen öffnen/schließen
Alle Rollladen hoch/runter

And others will interchangeably say „die Jalousie(n)“ instead of „Rolladen“

I'm currently using the HomeKit integration witha Homepod and I frequently ask/tell Siri this:

Wie ist die Außentemperatur? (What's the outside temperature? There is a sensor named "Außentemperatur")
Schlafzimmer Süd auf 20%. (Bedroom south to 20%. There is a cover named "Schlafzimmer Süd" but I don't say Rolladen/Jalousie.)
Schalte das Licht aus. (Turn off the lights. The Homepod knows it's in the bedroom and therefore only switched all lights in that room off)

For covers, I usually say:

Öffne/schließe den/die Rolladen in Pauls Zimmer
Öffne/schließe den/die Rolladen im Wohnzimmer
Öffne/schließe alle Rolladen
Rollladen auf x Prozent
Rollladen in Pauls Zimmer auf x Prozent
Alle Rollladen öffnen/schließen
Alle Rollladen hoch/runter

And others will interchangeably say „die Jalousie(n)“ instead of „Rolladen“

Alles außer x Prozent ist dann mit #92 abgedeckt.

 The percentage might depend on the covers then..

Additionally to "Schalte ein/aus" i also would say "Dreh(e) auf/ab".

Hi,
You should also take into account, that some would use "RollÄden" and some "RollAden" as the plural form of "Rolladen"

And for getTemperature I'd like to use:
Wie warm ist es (in/im) (der Küche/Schlafzimmer/draußen/oben etc.)?

Hi how proper should the grammar be and should we include slight dialect variations?
Because even tho according to the duden it is:

das Rollo

it sounds weird for me so I use:

die Rollo

Regarding dialects sometimes I say:

moch

instead of:

mach

Also regarding areas.
I often say:

(bei (der | die | den)  | beim) {area}
for children rooms.

Another variation for getting a temperature value:

"Wie viel Grad {Celsius | Fahrenheit} hat es ?"

Further more: Do we have other domains that needs to be added?
At the moment, I am running some Amazon Echos and have integrated different routines in the devices with which scripts will be triggered.
These will be used for changing settings on my EV Charging Station or start the charging of the EV...
But I could also imagine (for a wider adaptation) to controll TVs (turn on & set channel)...
Should we start with creating files for such examples, too? Or isn't that planned as of now?


Update release-it package to 14.12.3

Codecov Report

Merging #10 (031399c) into master (c0b1a4a) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #10   +/-   ##
=========================================
  Coverage   100.00%   100.00%
=========================================
  Files           12        12
  Lines          393       393
=========================================
  Hits           393       393

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0b1a4a...031399c. Read the comment docs.


Atoms rebased unsquashed

rebased/squashed/unsquashed version of #226, with tests for the cases mentioned in https://github.com/bustlelabs/mobiledoc-kit/pull/226#issuecomment-173630690
Ready for review
cc @mixonic @rlivsey

:+1: awesome stuff


[Feature] mamba.api?

conda provides a Python API to some of its internals, with "very long-term stability". This is useful for creating your own wrappers around conda environments (for instance, in our use-case we want to manage a "run" environment that is a subset of our overall "dev" environment), which we can do by hooking into conda.api.Solver.
Would there be any interest in providing a similar API for mamba? We'd also be alright with using subprocesses if the mamba CLI was considered "relatively stable" (e.g. by doing something similar to conda-lock to generate lockfiles).

Hi @alanhdu thanks for asking.
We do have an internal API that can be used quite easily: https://gist.github.com/wolfv/cd12bd4a448c77ff02368e97ffdf495a
This is already used in the conda-forge bots and in some other places. I am not sure that the above example will work right now (as there might have been some changes) but you can always look at mamba.py to compare.
However, I won't guarantee super longterm stability as of now as we're still in an early stage.
Let me know how this goes!


Das_Geek: Watch banyo.co.uk

Das_Geek requests the watch of the watch_keyword banyo\.co\.uk. See the MS search here and the Stack Exchange search here.

banyo\.co\.uk has been seen in 1 true positive, 0 false positives, and 0 NAAs.

Approved by Machavity in Charcoal HQ


Typos: test/SILOptimizer

Fix typos in test/SILOptimizer
This is one batch of many PRs fixing typos, see the tracking issue.

@eeckstein Thank you for review! I've reverted according to your suggestions.

@swift-ci smoke test


take kernel support for StringViewArray and BinaryViewArray

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
This is part of the larger project to implement StringViewArray -- see https://github.com/apache/arrow-rs/issues/5374
In https://github.com/apache/arrow-rs/pull/5481 we added support for StringViewArray and ByteViewArray.
This ticket tracks supporting StringViewArray and ByteViewArray in the take kernel: https://docs.rs/arrow/latest/arrow/compute/kernels/take/index.html
Describe the solution you'd like

Support StringViewArray and ByteViewArray  the take kernel
Add tests

Notes:

you should be able to follow the model of DictionaryArray as the idea is similar
Filtering should not copy any strings, it should just manipulate the fixed size views

Describe alternatives you've considered

Additional context

I want to try this.

label_issue.py automatically added labels {'arrow'} from #5602


unit tests fail for bigger project

After installing the package on symfony 2.8.49 and running phpunit test v6.5.8 , bugsnag/bugsnag-symfony: v1.4.0 every instance of phpunit test creates a new instance of bugsnag client which causes memory leaks and tests failing.
test fail message:
Trace:
Warning: include(/Users/project/vendor/symfony/symfony/src/Symfony/Component/Debug/Exception/ContextErrorException.php): failed to open stream: Too many open files in /Users/project/vendor/composer/ClassLoader.php on line 444
Warning: include(): Failed opening '/Users/project/vendor/composer/../symfony/symfony/src/Symfony/Component/Debug/Exception/ContextErrorException.php' for inclusion (include_path='.:/usr/local/Cellar/php<EMAIL_ADDRESS>in /Users/project/vendor/composer/ClassLoader.php on line 444
PHP Warning:  include(/Users/project/vendor/symfony/symfony/src/Symfony/Component/Debug/Exception/ContextErrorException.php): failed to open stream: Too many open files in /Users/project/vendor/composer/ClassLoader.php on line 444
PHP Warning:  include(): Failed opening '/Users/project/vendor/composer/../symfony/symfony/src/Symfony/Component/Debug/Exception/ContextErrorException.php' for inclusion (include_path='.:/usr/local/Cellar/php<EMAIL_ADDRESS>in /Users/project/vendor/composer/ClassLoader.php on line 444
Fatal error: Method PHPUnit\Framework\ExceptionWrapper::__toString() must not throw an exception, caught ErrorException: Warning: include(/Users/project/vendor/phpunit/phpunit/src/Util/Filter.php): failed to open stream: Too many open files in /Users/project/vendor/phpunit/phpunit/src/TextUI/ResultPrinter.php on line 0
PHP Fatal error:  Method PHPUnit\Framework\ExceptionWrapper::__toString() must not throw an exception, caught ErrorException: Warning: include(/Users/project/vendor/phpunit/phpunit/src/Util/Filter.php): failed to open stream: Too many open files in /Users/project/vendor/phpunit/phpunit/src/TextUI/ResultPrinter.php on line 0
Fatal error: Uncaught Error: Class 'Symfony\Component\Debug\Exception\FatalErrorException' not found in /Users/project/vendor/symfony/symfony/src/Symfony/Component/Debug/ErrorHandler.php:705
Stack trace:
#0 [internal function]: Symfony\Component\Debug\ErrorHandler::handleFatalError()
#1 {main}
thrown in /Users/project/vendor/symfony/symfony/src/Symfony/Component/Debug/ErrorHandler.php on line 705
PHP Fatal error:  Uncaught Error: Class 'Symfony\Component\Debug\Exception\FatalErrorException' not found in /Users/project/vendor/symfony/symfony/src/Symfony/Component/Debug/ErrorHandler.php:705

I recommend contacting<EMAIL_ADDRESS>with this - they will be able to assist you better. Thanks!


Azure COSMOSDB_ ENOBUFS ERROR

I am using node version 10.6.0 and connects to Azure COSMOS DB using 'MongoCllient'. Code as below:
index.js
 var MongodbConnectionString = process.env.COSMOS;

 server.post('/api/test/',(req,res,next) =>{

 var MongoClient = require('mongodb').MongoClient;

        MongoClient.connect(MongodbConnectionString, {
         useNewUrlParser: true
        }, function (err, client) {

 var db = client.db("DBname");
 //rest of the code
 }
});

Node application is hosted on azure app service and works fine. But after many http request i am getting error
MongoNetworkError: failed to connect to server [URI:PORT] on first connect [MongoNetworkError: connect ENOBUFS  - Local (undefined:undefined)]
If I restart the azure app service, application is up and running , maybe like after 2 days same issue popups and need to restart app service again.
How it could be resolved? Please help me out

Did this resolve your issue?

Try setting the database URL inside the code to use this parameter 'authSource', Something like this - mongodb://myUserName:example/databaseName?authSource=admin


Test-Vault fails.

Not sure why this fails.

The vault and extension work fine otherwise.

Here's the logic for it:
https://github.com/TylerLeonhardt/SecretManagement.LastPass/blob/main/SecretManagement.LastPass.Extension/SecretManagement.LastPass.Extension.psm1#L398-L409
what does lpass status return for you.

I have to run wsl lpass status in Windows and I get a "not logged in" message. Even though I can run Get-Secret and Get-SecretInfo from my lastpass vault.

I went into WSL and logged in to Lastpass. Then back in Windows Test-SecretVault works.  Your test code is odd because even though the test technically fails, I can still extract secrets from the vault. Or it would be helpful to have more verbose detail about what Test-SecretVault is actually doing.

Yeah I'm not 100% sure what Test-SecretVault should really be doing. I'm tempted to always return true but I want it to provide some value.

From what I can infer, the test can be anything you want. I would assume it is to ensure "integrity" which in this case I'd have to think means being able to read from the vault, or somehow verifying that the vault isn't corrupt. lpass status doesn't seem to offer much value.  Maybe verifying that lpass ls doesn't throw an error?

Yeah that's probably the best bet. Would you like to contribute that? I'm going to need to find a new maintainer eventually for this repo as I don't use LastPass anymore :(

I can try to give this a go. I'll have to get set up to submit PRs to the repo.


Fix bug where a users identities are not being set on init_metadata

This PR addresses a bug where the identities of a user were not being set on creation of an App due to the fact that SyncManager::init_metadata did not call update_identities for each persisted user.
Fixes: https://github.com/realm/realm-cocoa/issues/6647

Remember changelog.md update

@bmunkholm there's no changelog to update in the object store, SDK's will have to note the changes themselves by looking at the commits as they update to the new object store submodule.

@ironage yeah I figured and deleted my comment long before your note :-) (for some reason I thought it was realm-cocoa repo) :-)


Unspecified error when phone put to sleep during zeroconf scan

Been using the library with a recent project and very satisfied with how easy to implement it was.
However, in tightening things up recently, I've noticed that zeroconf seems to be responsible for two types of crashes in my app.

When the app is minimized as the user navigates to the home screen. Error is encountered on resuming the app.

When the phone is put to sleep by pressing the power button. Error once again encountered on resuming the app.

In both cases, the error is an 'Uncaught, unspecified "error" event. (10)' with the following log
Unhandled JS Exception: Uncaught, unspecified "error" event. (-72000)
2017-01-05 16:01:20.156716 Tavern[310:44322] [] nw_socket_get_input_frames recvmsg(fd 7, 1024 bytes): [57] Socket is not connected
2017-01-05 16:01:20.241348 Tavern[310:44322] [] nw_socket_get_input_frames recvmsg(fd 10, 1024 bytes): [57] Socket is not connected
2017-01-05 16:01:20.248243 Tavern[310:44322] [] nw_endpoint_handler_add_write_request [8.1 <IP_ADDRESS>:8081 failed socket-flow (satisfied)] cannot accept write requests
2017-01-05 16:01:20.249545 Tavern[310:43888] [] __tcp_connection_write_eof_block_invoke Write close callback received error: [22] Invalid argument

Both errors only occur when a react-native-zeroconf scan is actively running. Disabling zeroconf or stopping the scan prevents the errors.
I was able to work around the first type of crash by writing some code to stop the zeroconf scan when the app is moved to the background. This was easy enough to do with React Native AppState. However, it seems significantly trickier to write such a workaround for handling when the phone's screen is turned off, at least using pure React Native.
Any ideas as to how the library could be updated to prevent these errors when the app state is changed during a zeroconf scan?

Hum not sure actually, what does your hook look like exactly? It seems a bit related to #12.

Hey there,
Thanks for getting back so soon.
We're not doing anything fancy when starting the scan (which is what I assume you're referring to by 'hook'). In addition to registering some listeners (onScanStart, onScanStop, and onGetDevices), this function is called in componentDidMount():
function startZeroConfScan () {   try {   zeroconf.scan(type = 'http', protocol = 'tcp', domain = 'local.');   } catch (e) {parseZeroConfError(e)} }
With regard to the other issue, this morning I tried toggling the wifi on and off and didn't encounter any crashes. I also notice that the error with the phone coming back from sleep only happened about 50% of the time. I couldn't find any pattern as to what made it either work or fail, however.

An update:
I managed to fix the red screen crashes by including a handler for the error event.
The error log are still the same as above, including that [57] Socket is not connected error, which I think is important. I've been running this app on iOS 10.2 and socket issues when the app goes to the background in iOS 10 have been noted elsewhere: http://stackoverflow.com/questions/39743686/web-service-calls-no-longer-work-in-background-when-background-audio-mode-is-ena

Hum thanks for the update! Do you have this issue only on physical devices or are you also able to reproduce on simulator? Still not sure what I can do about it.

I've only seen the error on the one physical device I'm using to develop. I have a feeling based on that stack overflow thread that it may be specifically an iOS 10 issue.

I am having the same issue as described here and I do believe it is related to #12 as well.
I am doing a new scan on network change and/or when the app state is changed to "active". I can trigger it by being in the app, double tapping the home button and then choose the app again. If I do this many times quite fast the error eventually appears and the service that is supposed to be resolved is set to nil for some reason.
At the moment I have added a check: if (service != nil) before doing anything with the service and everything seem to work fine. Of course that is not really solving the problem. Do you guys have any suggestions on why the service is null?
@jdpigeon - why do you think it is related to the "socket is not connected" issue that lots of people got with iOS 10?

@pellepersson Landed a similar change in the latest 0.8.0 version. @SiteFlo and @jamsesso did the same thing in their forks too, and might be interested.

@jdpigeon Have you had the chance to try reproducing using 0.8.0?

Just started working on this project again, actually. I'll take a look when I'm in the office on Monday

Cool! Have a good weekend 😃

Still noticing the same problems, unfortunately.
If I don't stop the zeroconf scan before minimizing the app I'll get an Unhandled JS Exception: Uncaught, unspecified "error" event. (-72000).
I'm no longer so sure that my problems related to that iOS10 Socket issue since that error doesn't always pop up at the same time as the zeroconf one.
In any case, I have been able to work around the error by stopping the scan when the app is put to sleep and creating an error handler.

Hum, and what happens if you keep scanning with the error handler? I would assume the error get caught right?

Yup! Error gets caught.
Unfortunately, it looks like the scan stops working properly. Changes to the list of zeroconf devices aren't detected after the error's been thrown until the component is remounted

Hey! We are now doing a sanity check on the service to prevent some errors which might catch your issue. Do you mind trying that?

It's still happening on 0.8.3 ...

Same here, and it happens very randomly

I fixed calling stop() method of zeroconf inside "resolved" event, give it a try ;)

NSNetServicesUnknownError corresponds to the 72000 status code, which is not really helpful in itself. However, it's likely the OS is putting some of the app net services on pause while in the background and resuming them on foreground,  resulting in the error you guys are seeing.
The idea of @rdev is actually a good one. It might be possible to stop it seamlessly inside the module, but while waiting for that people should just pause and resume the scan themselves at the right times in order to avoid this.

@balthazar @jdpigeon I found a fix/solution to this, you can listen for the "AppState" and remove the listener when the app becomes inactive and re-add them when the app becomes active again.
Here's an example:
AppState.addEventListener('change', this.handleAppStateChange.bind(this));

 private appState: string = 'active';

  private handleAppStateChange(nextAppState: string): void {
    if (this.appState === 'active' && nextAppState === 'active') {
      return;
    }

    this.appState = nextAppState;

    if (nextAppState === 'active') {
      return this.zeroConf.addDeviceListeners();
    }

    return this.zeroConf.removeDeviceListeners();
  }

The first line in the function prevents the listener to be added twice (This triggers an error as-well).
Hope this helps 😄

Awesome, @DennisSnijder! That worked for me as well. This is the hook-based variation that I came up with:
const [appState, setAppState] = useState(AppState.currentState);
const previousAppState = usePrevious(appState);
const [rnZeroconf, setZeroConf] = useState<RNZeroconf | null>(null);

const handleAppStateChange = (state: AppStateStatus) => {
    setAppState(state);
};

useEffect(() => {
    AppState.addEventListener('change', handleAppStateChange);
    return () => AppState.removeEventListener('change', handleAppStateChange);
}, []);

useEffect(() => {
    if (appState === 'active' && previousAppState === 'active') {
        return;
    }

    if (appState === 'active') {
        rnZeroconf?.addDeviceListeners();
    }

    rnZeroconf?.removeDeviceListeners();
}, [appState, previousAppState, rnZeroconf]);

with helper:
function usePrevious<T>(value: T) {
  const ref = useRef<T>();
  useEffect(() => {
    ref.current = value;
  });
  return ref.current;
}


Fix

#153

@Eubanks688 you have been spamming this repo all weekend, I'm sorry but I have to ban you from posting any more.


Crash if IRI is not available for attachToTangle

I0607 10:11:59.632150 19383 api_json.cc:329] attachToTangle request failed.
*** Aborted at<PHONE_NUMBER> (unix time) try "date -d @1528366319" if you are using GNU date ***
PC: @                0x0 (unknown)
./run_hub.sh: line 20: 19373 Segmentation fault      (core dumped) sudo GLOG_logtostderr=true GLOG_v=3

Getting the same Error:
*** Aborted at<PHONE_NUMBER> (unix time) try "date -d @1533900174" if you are using GNU date *** PC: @                0x0 (unknown) Segmentation fault: 11


OCaml 4.08+ support

Thanks for getting this project moving. Right now it doesn't seem like it's possible to depend on this package in newer versions of OCaml. I had to add the following to my esy resolutions:
    "@baransu/graphql_ppx_re": {
      "source": "baransu/graphql_ppx_re:esy.json#c7fab6c",
      "override": {
        "build": "dune build -p graphql_ppx",
        "dependencies": {
          "@opam/ocaml-migrate-parsetree": "*",
          "@esy-ocaml/reason": "*",
          "refmterr": null
        }
      }
    }

I think these could get integrated in master. I don't know what refmterr is but it just doesn't build on OCaml 4.09 which is what I was testing, and removing it works fine. The esy.json also looks weird, and many of its dependencies are very outdated (e.g. Menhir, OMP). I can submit an enhancement PR if you want.

Menhir version was hardcoded because of failing 4.02 tests on Windows and I probably left it like this for current esy.json.
You’re far more experienced and understand how thinks should be. Would be amazing if you could send a PR! 🚀


build(android): remove build tools version

Setting buildToolsVersion is no longer recommended, it'll default to Android Gradle Plugin defaults. It makes DX worse by requiring multiple versions of build tools by various dependencies require various versions.

Note that even if the PR title is fixed to be conventional, single-commit merges typically take the commit which is still possibly non-conventional. Since we use semantic-release bot you must be careful with the merge message! It will probably need a little manual fixup

Thanks @mikehardy

That's was important tip, usually I the commit was taken from merge itself. It's a good point to make sure those commits also follow semantic release, will check if I can configure husky to avoid such issues

alternatively there is a mode in the checker so it will flag the thing for more attention:

          # When using "Squash and merge" on a PR with only one commit, GitHub
          # will suggest using that commit message instead of the PR title for the
          # merge commit, and it's easy to commit this by mistake. Enable this option
          # to also validate the commit message for one commit PRs.
          validateSingleCommit: true

https://github.com/amannn/action-semantic-pull-request/
I'm mobile right now or I'd PR the workflow yaml sorry :)

I made the change on GitHub, sorry 😇


Canvas ignores object-fit property

When using any object-fit setting, its being ignored and images are simply stretched to fill the space due to flexbox props.
.bt-create-post-images-container {
  min-width: 100%;
  background-color: #8f8f8f;
  height: 350px;
  overflow: hidden;
  display: flex;
  flex-direction: row;
  justify-content: flex-start;
  align-items: center;
}

.bt-create-post-image-container {
  background-color: #8f8f8f;
  height: 100%;
  overflow: hidden;
  display: flex;
  flex-direction: row;
  justify-content: center;
  align-items: center;
}

.bt-create-post-image-container > img {
  width: 100%;
  height: 100%;
  object-fit: cover;
  align-self: auto;
  vertical-align: middle;
}

Hi, any news/workaround on this issue? I really need the cover option; I could use a background-size cover (supposedly fixed in issue #288) but as the size of my div is undefined (100% width, 100% height),  it's not working even with the last alpha build. Disclaimer: i'm an artist following js classes these days so I hope the solution is not too obvious. I can see many uses for html2canvas in my practice so thanks a lot anyway!

Are there updates regarding this issue? Has anyone found a workaround?

Hello, just to let you know that I needed this and hacked it to work in my project. I provide the code which works for me below. However this is a hack, I don't have the knowledge to integrate this into your project in a more proper way:
    key: 'drawImage',
      value: function drawImage(image, source, destination) {
        /**START CUSTOM CODE**/

          var newWidth = 30;
          var newHeight = 30;
          var newX = destination.left;
          var newY = destination.top;

          // console.log(image, source, destination);
          if (source.width/destination.width > source.height/destination.height) {
              newWidth = destination.width;
              newHeight = source.height * (destination.width / source.width);
              newY = destination.top + (destination.height - newHeight) / 2;
          } else {
              newWidth = source.width * (destination.height / source.height);
              newHeight = destination.height;
              newX = destination.left + (destination.width - newWidth) / 2;
          }
        // console.log(newWidth, newHeight);

          this.ctx.drawImage(image, source.left, source.top, source.width, source.height,
            newX, newY,
            newWidth, newHeight);
          // destination.width,
          // destination.height * (source.height / source.width)
          //   destination.width, destination.height);

        /**END CUSTOM CODE**/
      }

Take LanderBeeuwsaert's solution (by downloading html2canvas.js so you can edit its code like they did) and replace
if (source.width/destination.width > source.height/destination.height) {
with
if (source.width/destination.width < source.height/destination.height) {
That should get you your desired results. It worked for me, at least.

Use version 1.0.0-alpha.9 and add a patch-package with @LanderBeeuwsaert code like this one html2canvas+1.0.0-alpha.9.patch:
diff --git a/node_modules/html2canvas/dist/html2canvas.js b/node_modules/html2canvas/dist/html2canvas.js
index 5f34081..66d1909 100644
--- a/node_modules/html2canvas/dist/html2canvas.js
+++ b/node_modules/html2canvas/dist/html2canvas.js
@@ -2443,7 +2443,22 @@ var CanvasRenderer = function () {
     }, {
         key: 'drawImage',
         value: function drawImage(image, source, destination) {
-            this.ctx.drawImage(image, source.left, source.top, source.width, source.height, destination.left, destination.top, destination.width, destination.height);
+            var newWidth = 30;
+            var newHeight = 30;
+            var newX = destination.left;
+            var newY = destination.top;
+
+            if (source.width/destination.width < source.height/destination.height) {
+                newWidth = destination.width;
+                newHeight = source.height * (destination.width / source.width);
+                newY = destination.top + (destination.height - newHeight) / 2;
+            } else {
+                newWidth = source.width * (destination.height / source.height);
+                newHeight = destination.height;
+                newX = destination.left + (destination.width - newWidth) / 2;
+            }
+
+            this.ctx.drawImage(image, source.left, source.top, source.width, source.height, newX, newY, newWidth, newHeight);
         }
     }, {
         key: 'drawShape',

Don't forget to import the version that it is not minified by providing the full path:
const html2canvas = require('html2canvas/dist/html2canvas.js');

FYI, there's a different solution/workaround, as I struggled with object-fit and object-position. Instead of using an img, use a div with following CSS:
background-image: url(<MY_SOURCE>);
background-size: <OBJECT_FIT>;
background-position: <OBJECT_POSITION>;

FYI, there's a different solution/workaround, as I struggled with object-fit and object-position. Instead of using an img, use a div with following CSS:
background-image: url(<MY_SOURCE>);
background-size: <OBJECT_FIT>;
background-position: <OBJECT_POSITION>;

The picture will be blurred

@clevergod Do you have a fix for using object-fit property?

@niklasvh Hi, I am using version 0.41 for the moment.
It includes "background-size:cover", but not "object-fit".
Any plan to fix it ?

FYI, there's a different solution/workaround, as I struggled with object-fit and object-position. Instead of using an img, use a div with following CSS:
background-image: url(<MY_SOURCE>);
background-size: <OBJECT_FIT>;
background-position: <OBJECT_POSITION>;

This works

FYI, there's a different solution/workaround, as I struggled with object-fit and object-position. Instead of using an img, use a div with following CSS:
background-image: url(<MY_SOURCE>);
background-size: <OBJECT_FIT>;
background-position: <OBJECT_POSITION>;

This works

No on iOS, background image will run into errors

@niklasvh do you think that a future patch will come for this bug ?

FYI, there's a different solution/workaround, as I struggled with object-fit and object-position. Instead of using an img, use a div with following CSS:
background-image: url(<MY_SOURCE>);
background-size: <OBJECT_FIT>;
background-position: <OBJECT_POSITION>;

Does not work when you need to enable tainted canvas and CORS


Clearly document whether a component (or example) is dependent on other SCSS files

Clearly document whether a component is dependent on other Bootstrap SCSS files in Bootstrap 4.x.
Some components are dependent on other SCSS Bootstrap classes for full or partial functionality. Document these clearly for each component, so that new users to Bootstrap 4.x can use this clearly.
The documentation on on importing SCSS classes is thin, and making many of the examples on the documentation website work involves troubleshooting imports.
Example use case:
When using the Navbar component in Bootstrap 4.x, the documentation on importing SCSS classes is woefully thin. Based purely on the instruction to look at bootstrap.scss's import order, one might assume based on filename alone that you would only need to import navbar in your custom SCSS file like so:
@import "bootstrap/scss/navbar";

The reality is that the Navbar class depends on a minimum of the following:
//Required
@import "bootstrap/scss/functions";
@import "bootstrap/scss/variables";
@import "bootstrap/scss/mixins";

@import "bootstrap/scss/transitions";

In most use cases, it also requires the following:
//If using dropdowns for submenus
@import "bootstrap/scss/dropdown";

// If right-aligning anything
@import "bootstrap/scss/utilities";

To make the first example on the docs page work with a copy-and-paste, you also need the following:
// Styles the searchbox

@import "bootstrap/scss/forms";
@import "bootstrap/scss/buttons";

Should we document this in the hosted docs, in the Sass files (comment at the top?), both, or something else?

In docs certainly, and what about supress @warn if I don't have specific imports ? or maybe Sass module system is capable to handle this https://github.com/twbs/bootstrap/issues/29853 ?

We'll get over to Dart Sass, but I'm not sure about the module system yet. That looked like it's be a bigger lift than we can take right now.

👋
Hi, I logged this because I'm actually a technical writer with the Cloud Native Computing Foundation :)
My suggestion for you, from a documentation/writing standpoint is to document it on each component's page: For example, for Navbar, I'd include a section called "Prerequisites" or "Dependencies", directly before or after "How it works" or after.
Something like this:

Dependencies
If you are importing Bootstrap modularly, you must import the following for Navbar to render correctly: (or smth along these lines)
@import blah
@import blah
@import navbar

How it works
.... //etc

WIP of doing something small in our Sass files to get started: https://github.com/twbs/bootstrap/pull/31744.

I have a similar but simpler request: list all of the files included in node_modules/bootstrap/scss/bootstrap on https://getbootstrap.com/docs/5.0/customize/sass/#importing ? I want to be able to use the functions and customize the styling. I tried just selecting the components I needed but I evidently missed some dependencies and had to go through the files in node_modules to grab more files until it worked. It would have been useful to have a list of everything.


⚠ Samsung has degraded performance

In f7e9a60, Samsung (https://samsung.com) experienced degraded performance:

HTTP code: 200
Response time: 2182 ms

Resolved: Samsung performance has improved in 6821e73.


rdar://problem/30059364 BuildEngine shouldn't call exit for error

handling that isn't necessarily a programmer error

Added delegate method for error propagation to BuildEngineDelegate
Added delegate function pointer for error propagation to llb_buildengine_delegate_t_
Adopted delegate implementations and tests accordingly
BuildEngine uses this to report errors instead of aborting

Note that one exit remains here: https://github.com/apple/swift-llbuild/blob/master/lib/Core/BuildEngine.cpp#L1077-L1082 as I wasn't sure how to supply an acceptable return value at that point. Should be negligible though as this wasn't mentioned in the original radar and still has a FIXME attached.

@swift-ci please test

@swift-ci Please test


Gift Name Missing

I am using a listener for gift events, but the names of some gifts are missing.
Here is an example from LOL.
"gift": {
  "id": 5748,
  "name": "",
  "diamondCost": 20,
  "picture": {
    "link": "https://storage.streamdps.com/iblock/874/87462af5f0cb390712032236f06535c8/761e6f552b038fb5fcd0b0b060b53f0c.png"
  },
  "properties": {
    "id": 5748,
    "name": "",
    "image": "https://storage.streamdps.com/iblock/874/87462af5f0cb390712032236f06535c8/761e6f552b038fb5fcd0b0b060b53f0c.png",
    "diamondCost": 20,
    "properties": {}
  }
},

Additionally, if there is a mapping table of gift IDs to names, I would appreciate it if you could provide it.

Solved.

Solved.


Avatarplaceholder does not load

Describe the bug
packages/template/public/_flowershow/avatarplaceholder.png (permalink) is stored as an LFS file which is not resolved by npm. Provisioning a new installation therefore provisions the file as LFS reference which a browser can't load.
To Reproduce
Set up a new Node installation
Set up a Flowershow installation
Preview or export so that the public folder exists on the file system.
Expected behavior
avatarplaceholder.png is stored as actual non-LFS png so that a browser can render it without manually replacing the file.
Additional context
I was asked by rufuspollock on Discord to open this.

Fixed with 2276420 and 4a5dc8c.
@regalialong Thank you for reporting this!


Save annotation on file before opening different image

I am using labelImg to annotate object for object annotation task. I found it quite useful but I noticed that when an annotation is modified and I switch to another image the annotation is lost. This is quite strange for a program feature.
The more intuitive approach would be to ask if changes should be saved and then proceed to opening the new image. Another alternative would be to automatically save changes if file already exist (annotation file is being edited and not created from scratch for example).

Windows 7
I am not sure which pyQt I have or if I have it(!).

I'm not experiencing this w/ the latest code, so I believe this issue has since been dealt w/.
@eypros Please pull the latest code or build.

I have now a better understanding of the program. So, on version 1.6 the problem is still present but in a specific situation:
If I move between images using the Next Image/Previous Image or their keyboard shortcut the warning exists (as was expected) BUT if I choose to pick an image by mouse it does not.
So, please take a look into that also.

Interesting, thanks for the update.

I'm also experiencing something like this.
In the last version of the software, I was able to use the "D" shortcut key to move to the next image. This would automatically save the annoation that was drawn.
However, in the latest version, when "D" is pressed a warning is displayed saying 'You have unsaved changes, would you like to save them and proceed? Click "No" to undo all changes'.
When drawing thousands of bounding boxes this feature really slows down the process.
Happy for this to be an optional setting because it may be useful for some users.


Update labels.json

Fixes #
Proposed Changes

Screenshots

The points in the label.json are labelled like the Fibonacci sequence I think therefor 4 would be not included.

Closing this PR hope this is OK for you feel free to reopen or create another one :)
__
  Summary: :ok_hand: This code looks good


[nexus] Runtime configuration of ICE candidates

Hi Everyone,
I'm running some experiments using the Nexus app in a Kubernetes environment. Since I'm using STUNner (basically a Kubernetes native TURN server) I needed to add my own TURN server as an ICE candidate.
Currently I think there is no way to change the ICE config during runtime, so I needed to fork the project then build my own Docker image using a specific TURN server. This worked fine, but it would be nice to have a way to configure the ICE candidate in runtime.
The are two files containing the ICE config (for client and server):

nexus/assets/js/home.js
nexus/lib/nexus/peer.ex

Usually .js files can be overwritten during runtime in Kubernetes using ConfigMaps, but this was not as obvious to do so, since the file itself is quite large, and there are also minimized and gzipped versions in the container. To that end, it would be nice if the ICE config would be in a separate .js file.
On the server side, I have zero experience with Elixir, so I have no idea how to use an ENV variable for this purpose (especially that TURN config requires more parameters, e.g. username and password).
If you also value in this feature, can you help me add this to Nexus (and maybe to the other projects as well)?

Hi @Megzo,
We'll be more than happy to make ICE servers configurable at runtime. Server-side (peer.ex), this should be pretty easy and we can take care of it, however, I'm not sure I understand how exactly you mean to configure it in the client-side code.
Extracting the ICE config to a separate .js file can be done ofc, but the minified and gzipped versions of the frontend assets are generated while building the Docker image. In the Dockerfile, this is done using mix assets.deploy which is an alias for these three tasks:

tailwind nexus --minify
esbuild nexus --minify
phx.digest

I'm not really that familiar with frontend development as a whole, so it's hard for me to say whether it's possible to somehow overwrite assets generated in this way at runtime. If you're determined to bring this feature without having to build your own Docker image, you can look into this topic and see if anyone has tried (and succeeded) to do something like this with Phoenix before.
Would you be willing to look for a potential solution to this issue?

Hi @sgfn
Thanks for the quick replay.
Sure, I'll look into how to organiz the .js files to that end.
I'll get back to you, when I have the idea.

With Broadcaster release 0.7.1, ICE servers are now configurable at runtime 🎉
The new ICE env variables are:

ICE_SERVER_URL: if unset, defaults to the google stun server
ICE_SERVER_USERNAME
ICE_SERVER_CREDENTIAL
ICE_TRANSPORT_POLICY: all by default, can be set to relay

These options allow users to specify one ICE server that will be used both by WHIP/WHEP web clients and the Broadcaster server


Add format method to supplement existing jpeg/png/webp methods

The format method would be a wrapper around the existing jpeg, png and webp (and forthcoming raw) methods.
It should accept enum-esque Object values (like sharp.gravity), but the values themselves could be Strings. For example both format(sharp.format.png) and format('png') would work.
See the discussion against https://github.com/lovell/sharp/pull/61 for the origins of this feature.

This new convenience method will be included in v0.9.2.


Multiple Extensions: Library token problems

This issue came out this:
https://github.com/migueldeicaza/SwiftGodot/issues/65
When SwiftGodot is initialized calling initializeSwiftModule we pass a pointer to the extension that Godot provides us.   This token is used in a few places in the library to carry out some work.
The ClassInfo registration API relies on this token, I suspect to associate the class information to this particular library, in case Godot wants to unload the library and reload it.
ClassInfo is used both directly by the user, but also implicitly by the signal registration system.
And lastly, the Wrapped code (the base class for Godot's Object) also consumes this token when associating object instances with this module.
Passing the correct token to these three places is problematic, lets explore the challenges:

ClassInfo creation: this is easy, we just need to keep around this token and reuse it.
Signal registation: this happens as a class constructor, so if the code is shared between two extensions, the signal proxy would only be registered to the first extension.   So we would need some way of registering the signals bound to the token, which typically happens via the connect method, so we would need to pass this handle there.
Object creation: every object we create ends up being register to this handle, when we call set_intance_binding and this one would be incredibly troublesome to pass around.

This is what other bindings do:

C++: uses static libraries, so while it duplicates all the code for each extension, it can rely on a global variable as its token.
Rust: it associates the token with the rust shared library, which is our current behavior, but I suspect wrong.
LibGodotSharp: uses a global, like Rust and us
Godot's builtin C# bindings: it is a bit dense and I have not been able to sort out exactly how it is used.

The options would be:

Do not support dynamic unloading
Find out a way that we could have libSwiftGodot.dylib find the call site dynamic library and use that as the token
Always produce static libraries for each module, and not rely on libSwiftGodot.dylib at all.

Option 2 is possible using #dsohandle. This provides a pointer to the dynamic object it is called from. The pointer can be converted to a UInt for Hash compat and directly using the pointer as an id. [UInt: GDExtensionClassLibraryPtr]
UInt(bitPattern: #dsohandle)

This code can be used to test the feature. I added it to a local branch. A call to it can be added in the init macro.
public func idTest(callerID: UnsafeRawPointer = #dsohandle) {
    print("GDExt: \(callerID)")
    print("LSGD: \(#dsohandle)")
}

// GDExt: 0x000000010f434000
// LSGD:  0x0000000138000000

My understanding is that using #dsohandle as a default will evaluate it at the call site.


Locale warning on Linux

Hi @jonmmease,
I get the following warning on linux:
[0710/121110.792499:WARNING:resource_bundle.cc(435)] locale_file_path.empty() for locale
Plot seems to generated just fine. Any ideas?
Regards,
Ioannis

Hi @igiagkiozis,
I've seen that too, and I haven't tracked down the cause.  In general, chromium writes several kinds of warning messages to standard-err that don't seem to cause any issues so I'd recommend capturing standard error of the subprocess so it's doesn't make its way to users.
What I do in the Python library is use a background thread to continually read standard error into a buffer. Then I only display it to the user if Kaleido initialization crashes (e.g. doesn't even return an error message on standard out), or if an image export request returns nothing on standard output (I've only seen this happen when passing a 200MB+ JSON bundle, when Chromium raises an internal error that the max message buffer size is exceeded).

Hi @jonmmease,
Yes, that did the trick, thanks!


Request - Ice Barrage Timer

Casting ice barrage with an ahrims staff (at least in LMS) shows 6 tick timer but it should only be 5 ticks.

the kodia wand in lms seems to be a tick too fast (4 instead of 5). Maybe this is because LMS items have different item IDs?

Fixed in 3e9e8e6e724465a63989276d90a70981ad39edf4; I'll update this again when it lands in Runelite main.


Adding Additional unit test template

Opening this PR so that we can get feedback on our unit test templates. What do you think about having 2-3 templates available? Are there any other major/minor changes that you think they need?

This change is

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

Tests.Template/unit_template1.ps1, line 2 [r1] (raw file):

<#
        .Synopsis

SYNOPSIS

Tests.Template/unit_template1.ps1, line 3 [r1] (raw file):

<#
        .Synopsis
        A sample template for creating DSC Resource Unit Tests mainly for smaller resources

Indent text below the help keywords

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

# TODO: Customize these parameters...
$script:DSCModuleName      = '<ModuleName>' # Example xNetworking
$script:DSCResourceName    = '<ResourceName>' # Example MSFT_xFirewall

These should be camel case ($script:dscModuleName, $script:dscResourceName)

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

#endregion HEADER

# TODO: Other Optional Init Code Goes Here...

We should use Pester's BeforeAll instead of putting init code here.
Norberto recently opened an issue for this in DscResources I believe.
That means a Describe block encapsulating all the tests would be needed.
Which means the template would look more like this:
Describe "$script:dscResourceName Unit Tests" {
BeforeAll {
resource-specific init code here
}
AfterAll {
    resource-specific cleanup code here
}

Context Get-TargetResource {
    ...
}
Context Set-TargetResource {
    ...
}
Context Test-TargetResource {
    ...
}

}
Though Pester may start complaining about script variables used in the Describe block that are declared outside the Describe block then.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

$script:DSCModuleName      = '' # Example xNetworking
$script:DSCResourceName    = '' # Example MSFT_xFirewall

What are those 2 variables used for? They are nor referenced by the template.

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):

-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests'))) -or `
(-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1'

Do we need the 2 checks here? Isn't the check for TestHelper.psm1 enough?

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> We should use Pester's BeforeAll instead of putting init code here.
> Norberto recently opened an issue for this in DscResources I believe.
> That means a Describe block encapsulating all the tests would be needed.
>
> Which means the template would look more like this:
> Describe "$script:dscResourceName Unit Tests" {
>     BeforeAll {
>         resource-specific init code here
>     }
>
>     AfterAll {
>         resource-specific cleanup code here
>     }
>
>     Context Get-TargetResource {
>         ...
>     }
>     Context Set-TargetResource {
>         ...
>     }
>     Context Test-TargetResource {
>         ...
>     }
> }
>
> Though Pester may start complaining about script variables used in the Describe block that are declared outside the Describe block then.

I think we will have to hold on on my suggestion of using Before/AfterAll. I wouldn't like to encourage people to use Context for the function name; Context should be used more to describe pre-conditions (scenario) for the tests.
I confirmed with the Pester team that support for nested Describes will come in 4.0. Then we can have a main Describe with Before/AfterAll for the entire test suite,

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 8 unresolved discussions.

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

#region Pester Test Initialization

# TODO: Optionally create any variables here for use by your tests
# See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1

#endregion Pester Test Initializati

We should probably wrap this in a function (BeforeAllTests?), which would make it easier to move this to a BeforeAll block when support for nested Describes are available.
Similar comment for the code in finally.

Tests.Template/unit_template1.ps1, line 82 [r1] (raw file):

    # TODO: Pester Tests for any non-exported Helper Cmdlets
    # If the resource does not contain any non-exported helper cmdlets then
    # this block may be safetly deleted.

"safely"

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):

    #region Example state 1
    Describe 'The system is not in the desired state' {

Context is more appropriate for this.
Describe 'Resource' {
	Context 'When the system is not in the desired state' {
		#TODO: Mock cmdlets here that represent the system not being in the desired state

		#TODO: Create a set of parameters to test your get/test/set methods in this state
		$testParameters = @{
			Property1 = 'value'
			Property2 = 'value'
		}

		#TODO: Update the assertions below to align with the expected results of this state
		It 'Get method returns something' {
			Get-TargetResource @testParameters | Should Be 'something'
		}

		It 'Test method returns false' {
			Test-TargetResource @testParameters | Should be $false
		}

		It 'Set method calls Demo-CmdletName' {
			Set-TargetResource @testParameters

			#TODO: Assert that the appropriate cmdlets were called
			Assert-MockCalled Demo-CmdletName
		}
	}
	Context 'When the system is in the desired state' {
		... etc ...
	}
}

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):

It 'Get method returns something'

oh! and It should use 'Should'
It 'The Get method should return something'

Comments from Reviewable

What are the differences between the templates in that case? What is a small resource?
My thought is that it would be better to use the same template as the base for all resource.

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> >  It 'Get method returns something'
>
> oh! and It should use 'Should'
>
>    It 'The Get method should return something'

I suggest we use `Describe "$($script:DSCResourceName)` or `Describe "$($script:DSCResourceName)\Get-TargetResource"`
Should we use BeforeAll like this?
    Context 'When the system is not in the desired state' {
        BeforeAll {
            #TODO: Mock cmdlets here that represent the system not being in the desired state
         }

It-blocks that test the Get-method could use It 'Should...' like this
            $result =  Get-TargetResource @testParameters

            It 'Should return the desired state as absent' {
                  $result.Ensure | Should Be 'Absent'
            }

            It 'Should return the same values as passed as parameters' {
                $result.MyProperty1 | Should Be 'something'
                $result.MyProperty2 | Should Be 'another thing'
            }

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):
Agree on the Describe label. 'Resource' was just a dummy example.
Agree on use of BeforeAll wrapping this code:

    #TODO: Mock cmdlets here that represent the system not being in the desired state

Quoted 5 lines of code…
>         #TODO: Create a set of parameters to test your get/test/set methods in this state
>         $testParameters = @{
>             Property1 = 'value'
>             Property2 = 'value'
>         }

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> > $script:DSCModuleName      = '' # Example xNetworking
> > $script:DSCResourceName    = '' # Example MSFT_xFirewall
>
> What are those 2 variables used for? They are nor referenced by the template.
>
>

There used to set up the Test environment, and for the titles of the Describe blocks mainly.

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> > -not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests'))) -or `
> >      (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1'
>
> Do we need the 2 checks here? Isn't the check for TestHelper.psm1 enough?

If the DSCResource.Tests folder doesn't exist than it would throw an error if we only do the one check. This code will be replaced in the near future anyway

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> I think we will have to hold on on my suggestion of using Before/AfterAll. I wouldn't like to encourage people to use Context for the function name; Context should be used more to describe pre-conditions (scenario) for the tests.
>
> I confirmed with the Pester team that support for nested Describes will come in 4.0. Then we can have a main Describe with Before/AfterAll for the entire test suite,

Okay, so should we leave it as is for now?

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> >     #region Pester Test Initialization
> >
> >     # TODO: Optionally create any variables here for use by your tests
> >     # See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1
> >
> >     #endregion Pester Test Initializati
>
> We should probably wrap this in a function (BeforeAllTests?), which would make it easier to move this to a BeforeAll block when support for nested Describes are available.
>
> Similar comment for the code in finally.

Done.

Tests.Template/unit_template1.ps1, line 82 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> "safely"

Done.

Comments from Reviewable

@johlju I agree that it would be easier to use the same template, however there are different styles of writing unit tests and some are much better than others depending on the nature of the functions in the resource

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> There used to set up the Test environment, and for the titles of the Describe blocks mainly.

I'd suggest  removing them to simplify the template and also because there is a $script:DSCResourceName  and a $global:DSCResourceName .
Also, I think  Initialize-TestEnvironment can infer these values from the filename.

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> If the DSCResource.Tests folder doesn't exist than it would throw an error if we only do the one check. This code will be replaced in the near future anyway

Who throws the error? Both Test-Path and Join-Path seem to work fine if the paths do not exist
PS> Test-Path c:\foo\bar
False
PS> Join-Path c:\foo\ bar
c:\foo\bar

The intention here being simplifying the template. Having a single check improves readability.

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Okay, so should we leave it as is for now?

Yes, just with the change below to wrap this is a function.

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Done.

Thanks

Tests.Template/unit_template1.ps1, line 82 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Done.

Thanks

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template1.ps1, line 2 [r1] (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> SYNOPSIS

Done.

Tests.Template/unit_template1.ps1, line 3 [r1] (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Indent text below the help keywords

Done.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> These should be camel case ($script:dscModuleName, $script:dscResourceName)

Done.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> I'd suggest  removing them to simplify the template and also because there is a $script:DSCResourceName  and a $global:DSCResourceName .
>
> Also, I think  Initialize-TestEnvironment can infer these values from the filename.

Okay, so what are you suggesting we replace them with since $script:dscResourceName is being used throughout the file

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> Who throws the error? Both Test-Path and Join-Path seem to work fine if the paths do not exist
>
>     PS> Test-Path c:\foo\bar
>     False
>     PS> Join-Path c:\foo\ bar
>     c:\foo\bar
>
> The intention here being simplifying the template. Having a single check improves readability.

Okay, I can remove it

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> Yes, just with the change below to wrap this is a function.

Done.

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Done.

Done.

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> Agree on the Describe label. 'Resource' was just a dummy example.
>
> Agree on use of BeforeAll wrapping this code:
>
> >         #TODO: Mock cmdlets here that represent the system not being in the desired state
> >
> >         #TODO: Create a set of parameters to test your get/test/set methods in this state
> >         $testParameters = @{
> >             Property1 = 'value'
> >             Property2 = 'value'
> >         }
>

Done.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Okay, so what are you suggesting we replace them with since $script:dscResourceName is being used throughout the file

I'd suggest removing them altogether
# TODO: Customize these parameters...

$TestEnvironment = Initialize-TestEnvironment `
     -DSCModuleName  '<ModuleName>'`
    -DSCResourceName '<ResourceName>' `
    -TestType Unit

$script:dscResourceName is not being used throughout the file. $global:dscResourceName is. They are different variables.
Also, our convention for naming the test file is .Tests.ps1, correct?  (or is it ..Tests.ps1?)  No need to have the test author specify those. Initialize-TestEnvironment can grab them (or at least one of them, depending on our convention) from the filename,

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Done.

thanks

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

Tests.Template/unit_template2.ps1, line 100 [r3] (raw file):

        #region Example state 2
        Context 'When the system is in the desired state' {
            #TODO: Mock cmdlets here that represent the system being in the desired state

Sjoudl use BeforeAll{] here to like above to be consistent.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

try
{
    Initialize-PesterTests

It's not obvious that this function should be used  to add init code. When your read it it feels like a function for the initialization of the pester tests for the template. Unless you read to the bottom.
A suggestion is that we add a comment like # In this function, at the end of the script, you can add your own optional init code

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> It's not obvious that this function should be used  to add init code. When your read it it feels like a function for the initialization of the pester tests for the template. Unless you read to the bottom.
> A suggestion is that we add a comment like `# In this function, at the end of the script, you can add your own optional init code`

Agree.
BeforeAllTests?

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 17 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> I'd suggest removing them altogether
>
>     # TODO: Customize these parameters...
>
>     $TestEnvironment = Initialize-TestEnvironment `
>          -DSCModuleName  ''`
>         -DSCResourceName '' `
>         -TestType Unit
>
> $script:dscResourceName is not being used throughout the file. $global:dscResourceName is. They are different variables.
>
> Also, our convention for naming the test file is .Tests.ps1, correct?  (or is it ..Tests.ps1?)  No need to have the test author specify those. Initialize-TestEnvironment can grab them (or at least one of them, depending on our convention) from the filename,

I see $script:dscResourceName being used in multiple places in the template. It makes sense to have variable for that and initialize it at the beginning, so I suggest leaving it as it is.

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

TODO: Optionally create any variables here for use by your tests
# See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1

These two lines should be removed since we call function to initialize tests

Tests.Template/unit_template1.ps1, line 19 [r3] (raw file):

$script:dscModuleName      = '<ModuleName>' # Example xNetworking
$script:dscResourceName    = '<ResourceName>' # Example MSFT_xFirewall
# /TODO

let's remove these # /TODO  closing tags.. they don't add any value (it's quite clear what TODO refers to) and it's just one more thing to remove before committing

Tests.Template/unit_template1.ps1, line 27 [r3] (raw file):

if (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1')))
{
    & git @('clone','https://github.com/PowerShell/DscResource.Tests.git',(Join-Path -Path $script:moduleRoot -ChildPath '\DSCResource.Tests\'))

We should not use git in our test templates: https://github.com/PowerShell/DscResources/issues/107

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> Agree.
>
> BeforeAllTests?

Invoke-TestSetup? or Initialize-TestSetup (to be consistent with Initialize-TestEnvironment)

Tests.Template/unit_template1.ps1, line 101 [r3] (raw file):

}

function Complete-PesterTests {

We should rename it...
my proposition would be Invoke-TestCleanup, but that'll depend what we rename Initialize-PesterTests to

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

<#
    .SYNOPSIS
       A sample template for creating DSC Resource Unit Tests mainly for larger resources

I'm not sure what does "larger resources" mean here...
This seems as if it's just a slightly more advanced version of the unit_template1 and we could easily add the simplified approach from that first template here in just another region (#Region example 3)... perhaps better than maintaining two templates which would only cause confusion?

Tests.Template/unit_template2.ps1, line 19 [r3] (raw file):

$script:dscModuleName      = '<ModuleName>' # Example xNetworking
$script:dscResourceName    = '<ResourceName>' # Example MSFT_xFirewall
# /TODO

Please remove /TODO

Tests.Template/unit_template2.ps1, line 27 [r3] (raw file):

if (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1')))
{
    & git @('clone','https://github.com/PowerShell/DscResource.Tests.git',(Join-Path -Path $script:moduleRoot -ChildPath '\DSCResource.Tests\'))

We should not use git  https://github.com/PowerShell/DscResources/issues/107

Tests.Template/unit_template2.ps1, line 46 [r3] (raw file):

    Initialize-PesterTests

    #region Pester Test Initialization

This region can be removed after adding Initialize-PesterTests

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 17 unresolved discussions.

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Okay, I can remove it

Done.

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> >  # TODO: Optionally create any variables here for use by your tests
> >     # See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1
> These two lines should be removed since we call function to initialize tests

Done.

Tests.Template/unit_template1.ps1, line 19 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> let's remove these # /TODO  closing tags.. they don't add any value (it's quite clear what TODO refers to) and it's just one more thing to remove before committing

Done.

Tests.Template/unit_template2.ps1, line 46 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> This region can be removed after adding Initialize-PesterTests

Done.

Tests.Template/unit_template2.ps1, line 100 [r3] (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Sjoudl use `BeforeAll{]` here to like above to be consistent.

Done.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> I see $script:dscResourceName being used in multiple places in the template. It makes sense to have variable for that and initialize it at the beginning, so I suggest leaving it as it is.

 $global:dscResourceName is being used, not $script:dscResourceName

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> Invoke-TestSetup? or Initialize-TestSetup (to be consistent with Initialize-TestEnvironment)

We are thinking about replacing this with BeforeAll once Pester supports nested Describes (V 4.0).
Not a big issue, though

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
>  $global:dscResourceName is being used, not $script:dscResourceName

That should have been taken out... do you have the most updated version?

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> We are thinking about replacing this with BeforeAll once Pester supports nested Describes (V 4.0).
>
> Not a big issue, though

Doesn't the function still need to follow the verb-noun format though?

Tests.Template/unit_template2.ps1, line 19 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> Please remove /TODO

Done.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Doesn't the function still need to follow the verb-noun format though?

I'd follow Pester's conventions

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> That should have been taken out... do you have the most updated version?

OK, I see the change.
My next question :) is now: why do we need a global variable? both the resource  and module names are part of the test environment returned by Initialize-TestEnvironment. Why not fetch them from there?

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> OK, I see the change.
>
> My next question :) is now: why do we need a global variable? both the resource  and module names are part of the test environment returned by Initialize-TestEnvironment. Why not fetch them from there?
>

There shouldn't be any globals in the unit tests. just script variables

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> There shouldn't be any globals in the unit tests. just script variables

My bad, but same question: why do we even need a script variable? Thanks

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 12 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> We are thinking about replacing this with BeforeAll once Pester supports nested Describes (V 4.0).
>
> Not a big issue, though

Definitely, we should use BeforeAll once it's available, but for now I suggest renaming Initialize-PesterTests and Complete-PesterTests to Invoke-TestSetup and Invoke-TestCleanup respectively

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> I'm not sure what does "larger resources" mean here...
> This seems as if it's just a slightly more advanced version of the unit_template1 and we could easily add the simplified approach from that first template here in just another region (#Region example 3)... perhaps better than maintaining two templates which would only cause confusion?

Saw @johlju raised the same concern before. Fully agree.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> Saw @johlju raised the same concern before. Fully agree.

I agree with the comments that @karolkaczmarek has specified. What is the basis for determining what is a "large resource"?

Tests.Template/unit_template2.ps1, line 43 [r4] (raw file):

{

    Initialize-PesterTests

Noun in PowerShell are singular. This should be Initialize-PesterTest

Comments from Reviewable

Reviewed 1 of 3 files at r1, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> Definitely, we should use BeforeAll once it's available, but for now I suggest renaming Initialize-PesterTests and Complete-PesterTests to Invoke-TestSetup and Invoke-TestCleanup respectively

I agree with the rename. Let's follow the approved verb-noun rules.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, nanalakshmanan (Nana Lakshmanan) wrote…
> I agree with the comments that @karolkaczmarek has specified. What is the basis for determining what is a "large resource"?

If you want input from a newbie of writing Pester tests (me) I vote for one template that are informative than several and I don't know which to choice. Then there won't be a concern If I choose wrong template for the resource I'm writing.
In that sense it's better to remove stuff from a template than add.

Tests.Template/unit_template2.ps1, line 60 [r4] (raw file):

    #region Example state 1
    Describe "$($script:dscResourceName)" {

In template number one, the describe block has "$($script:dscResourceName)\Get-TargetResource". But not template number 2.
Do we want to have it grouped differently? I would say not. I know we are discussing one or more templates. But if we are gonna go with more than one template I think that the output of the tests, regardless of template, should be the same to easily read the output after all the test have ran. It's like we have a style guide for code. DOn't we need a style guide for tests output too?

Comments from Reviewable

Review status: all files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> I agree with the rename. Let's follow the approved verb-noun rules.

Done.

Tests.Template/unit_template2.ps1, line 43 [r4] (raw file):
Previously, nanalakshmanan (Nana Lakshmanan) wrote…
> Noun in PowerShell are singular. This should be ```Initialize-PesterTest```

Done.

Comments from Reviewable

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> If you want input from a newbie of writing Pester tests (me) I vote for one template that are informative than several and I don't know which to choice. Then there won't be a concern If I choose wrong template for the resource I'm writing.
> In that sense it's better to remove stuff from a template than add.

I'm going to leave the SYNOPSIS comment as-is until we make a decision on how we want to format this. @johlju thanks for your feedback! Should we come up with some hybrid of these two templates then? Because I see pros and cons in each and the 'better' one I think depends on the format/size/number of functions in the resource.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> I'm going to leave the SYNOPSIS comment as-is until we make a decision on how we want to format this. @johlju thanks for your feedback! Should we come up with some hybrid of these two templates then? Because I see pros and cons in each and the 'better' one I think depends on the format/size/number of functions in the resource.

I suggest having the first, simpler template as the main (and only) one and include alternative structure of "describe" from the second, more advanced template as a comment with some explanation when we could use that instead. This ways out-of-the-box test template is as simple as possible for first-time users but provides more advanced structure for people who already have some experience with developing DSC resources and writing tests for those. Alternatively, we could adopt more advanced template alltogether - it covers superset of scenarios.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> I suggest having the first, simpler template as the main (and only) one and include alternative structure of "describe" from the second, more advanced template as a comment with some explanation when we could use that instead. This ways out-of-the-box test template is as simple as possible for first-time users but provides more advanced structure for people who already have some experience with developing DSC resources and writing tests for those. Alternatively, we could adopt more advanced template alltogether - it covers superset of scenarios.

@karolkaczmarek I agree. That sounds like the best solution. Especially since the templates are more for less experienced contributors

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> @karolkaczmarek I agree. That sounds like the best solution. Especially since the templates are more for less experienced contributors

Any progress on this?

Comments from Reviewable

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> Any progress on this?

I was working on releasing WebAdminDsc, so I haven't had time to update this yet. We're also now discussing the issue of using [InModuleScope](https://github.com/PowerShell/DscResources/issues/175). So I think we should incorporate a solution for that in our template as well

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> I was working on releasing WebAdminDsc, so I haven't had time to update this yet. We're also now discussing the issue of using [InModuleScope](https://github.com/PowerShell/DscResources/issues/175). So I think we should incorporate a solution for that in our template as well

I'd suggest getting rid of second template as this seems the last open issue here and getting this one merged in, so we have a base to work with.
The InModuleScope and not using git can be added in subsequent PRs, cause they require separate discussion and thorough review.

Comments from Reviewable

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> I'd suggest getting rid of second template as this seems the last open issue here and getting this one merged in, so we have a base to work with.
>
> The InModuleScope and not using git can be added in subsequent PRs, cause they require separate discussion and thorough review.

Okay, so get rid of unit_template2.ps1?

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 15 unresolved discussions.

Tests.Template/unit_template.ps1, line 18 [r1] (raw file):
Previously, narrieta (Norberto Arrieta) wrote…
> My bad, but same question: why do we even need a script variable? Thanks

They are used e.g. in "Describe" sections

Tests.Template/unit_template.ps1, line 51 [r5] (raw file):

    #region Function Get-TargetResource

The second template is now removed but it can't be found anywhere now. It's still valuable in certain scenarios, I'd suggest adding it to this template as a optional (commented out) section with information when it should be used instead.
Just make sure it's clear that part should be removed if we go with the simpler (default) version.

Comments from Reviewable

@kwirkykat - can you take a look at the most recent changes I made?

I like this setup with the patterns in test guidelines much better :+1:

Reviewed 1 of 3 files at r1, 2 of 3 files at r5, 1 of 2 files at r6.
Review status: 1 of 2 files reviewed at latest revision, 13 unresolved discussions.

TestsGuidelines.md, line 93 [r6] (raw file):

-------------

```powershell

All these region definitions are not needed.
Can this be more in the style of documentation rather than code comments?
Sorry that's kinda vague.
I mean more like this:
Pattern 1:
The goal of this pattern should be to describe the potential states a system could be in for each function.
Describe 'Get-TargetResource' {
        # TODO: Complete Get-TargetResource Tests...
}
Describe 'Set-TargetResource' {
        # TODO: Complete Set-TargetResource Tests...
}
Describe 'Test-TargetResource' {
        # TODO: Complete Test-TargetResource Tests...
}

Pattern 2:
The goal of this pattern should be to describe the potential states a system could be in so that the get/set/test cmdlets can be tested in those states. Any mocks that relate to that specific state can be included in the relevant Describe block. For a more detailed description of this approach please review https://github.com/PowerShell/DscResources/issues/143

Add as many of these example 'states' as required to simulate the scenarions that the DSC resource is designed to work with, below a simple 'is in desired state' and 'is not in desired state' are used, but there may be more complex combinations of factors, depending on how complex your resource is.
Describe 'The system is not in the desired state' {
    #TODO: Mock cmdlets here that represent the system not being in the desired state

    #TODO: Create a set of parameters to test your get/set/test methods in this state
    $testParameters = @{
        Property1 = 'value'
        Property2 = 'value'
    }

    #TODO: Update the assertions below to align with the expected results of this state
    It 'Should return something' {
        Get-TargetResource @testParameters | Should Be 'something'
    }

    It 'Should return false' {
        Test-TargetResource @testParameters | Should be $false
    }

    It 'Should call Demo-CmdletName' {
        Set-TargetResource @testParameters

        #TODO: Assert that the appropriate cmdlets were called
        Assert-MockCalled Demo-CmdletName
    }
}

or
 ```powershell
Describe 'The system is in the desired state' {
        #TODO: Mock cmdlets here that represent the system being in the desired state

        #TODO: Create a set of parameters to test your get/set/test methods in this state
        $testParameters = @{
            Property1 = 'value'
            Property2 = 'value'
        }

        #TODO: Update the assertions below to align with the expected results of this state
        It 'Should return something' {
            Get-TargetResource @testParameters | Should Be 'something'
        }

        It 'Should return true' {
            Test-TargetResource @testParameters | Should be $true
        }
}

Tests.Template/unit_template.ps1, line 51 [r5] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> The second template is now removed but it can't be found anywhere now. It's still valuable in certain scenarios, I'd suggest adding it to this template as a optional (commented out) section with information when it should be used instead.
> Just make sure it's clear that part should be removed if we go with the simpler (default) version.

It's been moved to the test guidelines because we don't want to clutter this file with large blocks of commented-out code.

Tests.Template/unit_template.ps1, line 13 [r6] (raw file):

        There are multiple methods for writing unit tests. This template provides a few examples
        which you are welcome to follow but depending on your resource, you may want to
        design it differently.

Can we add a link to the test guidelines in the NOTES section here?

Comments from Reviewable

Review status: 1 of 2 files reviewed at latest revision, 13 unresolved discussions.

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Okay, so get rid of unit_template2.ps1?

Done.

Comments from Reviewable

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

TestsGuidelines.md, line 93 [r6] (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> All these region definitions are not needed.
>
> Can this be more in the style of documentation rather than code comments?
> Sorry that's kinda vague.
> I mean more like this:
>
> Pattern 1:
> The goal of this pattern should be to describe the potential states a system could be in for each function.
>
> ```powershell
> Describe 'Get-TargetResource' {
>         # TODO: Complete Get-TargetResource Tests...
> }
> Describe 'Set-TargetResource' {
>         # TODO: Complete Set-TargetResource Tests...
> }
> Describe 'Test-TargetResource' {
>         # TODO: Complete Test-TargetResource Tests...
> }
> ```
>
> Pattern 2:
> The goal of this pattern should be to describe the potential states a system could be in so that the get/set/test cmdlets can be tested in those states. Any mocks that relate to that specific state can be included in the relevant Describe block. For a more detailed description of this approach please review https://github.com/PowerShell/DscResources/issues/143
>
> Add as many of these example 'states' as required to simulate the scenarions that the DSC resource is designed to work with, below a simple 'is in desired state' and 'is not in desired state' are used, but there may be more complex combinations of factors, depending on how complex your resource is.
>
> ```powershell
> Describe 'The system is not in the desired state' {
>     #TODO: Mock cmdlets here that represent the system not being in the desired state
>
>     #TODO: Create a set of parameters to test your get/set/test methods in this state
>     $testParameters = @{
>         Property1 = 'value'
>         Property2 = 'value'
>     }
>
>     #TODO: Update the assertions below to align with the expected results of this state
>     It 'Should return something' {
>         Get-TargetResource @testParameters | Should Be 'something'
>     }
>
>     It 'Should return false' {
>         Test-TargetResource @testParameters | Should be $false
>     }
>
>     It 'Should call Demo-CmdletName' {
>         Set-TargetResource @testParameters
>
>         #TODO: Assert that the appropriate cmdlets were called
>         Assert-MockCalled Demo-CmdletName
>     }
> }
>
> or
>  ```powershell
> Describe 'The system is in the desired state' {
>         #TODO: Mock cmdlets here that represent the system being in the desired state
>
>         #TODO: Create a set of parameters to test your get/set/test methods in this state
>         $testParameters = @{
>             Property1 = 'value'
>             Property2 = 'value'
>         }
>
>         #TODO: Update the assertions below to align with the expected results of this state
>         It 'Should return something' {
>             Get-TargetResource @testParameters | Should Be 'something'
>         }
>
>         It 'Should return true' {
>             Test-TargetResource @testParameters | Should be $true
>         }
> }
> ```

Done.

Tests.Template/unit_template.ps1, line 13 [r6] (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Can we add a link to the test guidelines in the NOTES section here?

Done.

Comments from Reviewable

Reviewed 1 of 3 files at r5, 1 of 2 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

Tests.Template/unit_template.ps1, line 27 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> We should not use git in our test templates: https://github.com/PowerShell/DscResources/issues/107

This can be fixed at a later date along with #107. It's not a priority at the moment.

Tests.Template/unit_template2.ps1, line 27 [r3] (raw file):
Previously, KarolKaczmarek (Karol Kaczmarek) wrote…
> We should not use git  https://github.com/PowerShell/DscResources/issues/107

This can be fixed at a later date along with #107. It's not a priority at the moment.

Tests.Template/unit_template2.ps1, line 60 [r4] (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> In template number one, the describe block has "$($script:dscResourceName)\Get-TargetResource". But not template number 2.
> Do we want to have it grouped differently? I would say not. I know we are discussing one or more templates. But if we are gonna go with more than one template I think that the output of the tests, regardless of template, should be the same to easily read the output after all the test have ran. It's like we have a style guide for code. DOn't we need a style guide for tests output too?
>

Ideally, yes. But there's currently a debate about whether the tests should be written by function or by machine state (that's why we are providing both patterns right now). *Technically* unit tests should be by function, but that can be very inconvenient for some of our tests where we repeat a mocked machine state for every function. I would prefer not making contributors rewrite tests to the other format right now if we're on the fence about which one we should use,but I don't see a common format that the Describe blocks can use if we allow both patterns, so for now we will just have to live with the inconsistent Describe block names across different tests.
We're going to have to come back to these templates when Pester has released nested Describe blocks so we can also remove the InModuleScope. I think when that happens we can re-visit what to do about this issue.
The Context blocks should always describe the 'context'/environment/specific state they set up.
The It blocks at least should always have a common format. They should all be named 'Should...' and describe the functionality that the It is testing such as 'Should return false when package is not installed' under a 'Test-TargetResource' Describe block.

Comments from Reviewable


Namespace 3.0 table names

Additionally, update references to table names to use either the correct new names, or an alias that is version-agnostic, depending.
Pivotal story: 107858710

I have one word: "boooring" :sleeping:

LGTM

Closing, but this work has been merged into #111.